Expose 'enable' property to allow the user to disable the SearchBar #137388
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie or stuartmorgan on the #hackers channel in Chat (don't just cc them here, they won't see it! Use Discord!). If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
There was a problem hiding this comment.
Thanks a lot for the contribution!
The SearchBar also has a InkWell to show different effects when hover, focused, pressed etc. So if we only disable the text field, we still can see the effects in these states as if the search bar is still enabled. This behavior might look a little strange.
To show a disabled effect for InkWell, we might also need to handle MaterialStates when the search bar is disabled:)
|
saw this behavior when testing the PR and it was strange. Thanks for pointing where i should look for |
|
This pull request executed golden file tests, but it has not been updated in a while (20+ days). Test results from Gold expire after as many days, so this pull request will need to be updated with a fresh commit in order to get results from Gold. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
|
@QuncCccccc Ready for the review. |
There was a problem hiding this comment.
Thanks for the fix! As the issue #136943 mentioned, search bar should be greyed out so users can notice it is disabled. We currently don't have a disabled spec from Material Design but usually we use opacity 0.38 to "disable" widgets(such as a disabled Card). So we can use Opacity and do:
return Opacity(
opacity: widget.enabled? 1 : 0.38,
child: ConstrainedBox(...)
)
Does this make sense?
|
Ok. I can add this. |
|
Everything except this disabled color is ok? |
Yep! Overall this looks good to me:) We can also check the opacity in the unit test. As for the disabled opacity number 0.38, could you also put this as a constant at the top of the file |
|
@QuncCccccc added the opacity and the test. Can you review, please? |
| textInputAction: widget.textInputAction, | ||
| keyboardType: widget.keyboardType, | ||
| child: Opacity( | ||
| opacity: widget.enabled ? 1 : _kDisableSearchBarOpacity, |
There was a problem hiding this comment.
Super nit: 1.0 instead of 1.
|
@Macacoazul01 Could you rebase master to trigger the tests again? I don't think the test failures are related to your change. |
|
Finaly |
flutter/flutter@9f2e681...7dc856a 2024-01-12 36861262+QuncCccccc@users.noreply.github.com Revert "Reverts "Run iOS staging tests with Xcode 15.2"" (flutter/flutter#141420) 2024-01-12 engine-flutter-autoroll@skia.org Roll Packages from 0744fe6 to d74d687 (5 revisions) (flutter/flutter#141449) 2024-01-12 tessertaha@gmail.com Fix `FlexibleSpaceBar` centered title position and title color (flutter/flutter#140883) 2024-01-12 whesse@google.com Do not reset framework checkout before running customer tests (flutter/flutter#141013) 2024-01-12 43054281+camsim99@users.noreply.github.com Increase delay for checking integration_ui_keyboard_resize test success (flutter/flutter#141301) 2024-01-12 godofredoc@google.com Add osx_sdk context for mac builds. (flutter/flutter#141422) 2024-01-12 engine-flutter-autoroll@skia.org Roll Flutter Engine from ecdaed76f284 to 44a0a6ee4d39 (18 revisions) (flutter/flutter#141432) 2024-01-12 barpac02@gmail.com Add support for Gradle Kotlin DSL (flutter/flutter#140744) 2024-01-12 36861262+QuncCccccc@users.noreply.github.com Fix typo (flutter/flutter#141426) 2024-01-11 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Run iOS staging tests with Xcode 15.2" (flutter/flutter#141412) 2024-01-11 15619084+vashworth@users.noreply.github.com Run iOS staging tests with Xcode 15.2 (flutter/flutter#141392) 2024-01-11 tessertaha@gmail.com Fix `ListWheelScrollView` in an `AnimatedContainer` with zero height throw an error (flutter/flutter#141372) 2024-01-11 andrewrkolos@gmail.com make asset_test.dart tests not dependent on context (flutter/flutter#141331) 2024-01-11 57464965+Macacoazul01@users.noreply.github.com Expose 'enable' property to allow the user to disable the SearchBar (flutter/flutter#137388) 2024-01-11 jonahwilliams@google.com Add impeller key to skia gold client, Turn on a framework test shard that will run unit tests with --enable-impeller (flutter/flutter#141341) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC camillesimon@google.com,rmistry@google.com,stuartmorgan@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
|
@HansMuller will this land on the next stable? |
|
Yes, it should be part of the next stable release. |
This exposes the
enabledproperty of theTextFieldwidget to theSearchbarwidget.Related Issues
#136943
Pre-launch Checklist
///).Still missing tests