Skip to content

Expose 'enable' property to allow the user to disable the SearchBar #137388

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
auto-submit merged 22 commits into from
Jan 11, 2024
Merged

Expose 'enable' property to allow the user to disable the SearchBar #137388

auto-submit merged 22 commits into from
Jan 11, 2024

Conversation

@Macacoazul01
Copy link
Contributor

@Macacoazul01 Macacoazul01 commented Oct 27, 2023 •
edited
Loading

This exposes the enabled property of the TextField widget to the Searchbar widget.

Related Issues

#136943

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I signed the CLA.
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

Still missing tests

@flutter-dashboard
Copy link

flutter-dashboard bot commented Oct 27, 2023

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.

@github-actions github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. labels Oct 27, 2023
@HansMuller HansMuller requested a review from QuncCccccc November 1, 2023 18:25
QuncCccccc
QuncCccccc reviewed Nov 2, 2023
View reviewed changes
Copy link
Contributor

@QuncCccccc QuncCccccc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:)

@Macacoazul01
Copy link
Contributor Author

Macacoazul01 commented Nov 2, 2023

saw this behavior when testing the PR and it was strange. Thanks for pointing where i should look for

@flutter-dashboard
Copy link

flutter-dashboard bot commented Nov 22, 2023

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 package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@Macacoazul01
Copy link
Contributor Author

Macacoazul01 commented Nov 25, 2023

@QuncCccccc Ready for the review.

QuncCccccc
QuncCccccc reviewed Dec 12, 2023
View reviewed changes
Copy link
Contributor

@QuncCccccc QuncCccccc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@Macacoazul01
Copy link
Contributor Author

Macacoazul01 commented Dec 12, 2023

Ok. I can add this.
When are you able to review again?

@Macacoazul01
Copy link
Contributor Author

Macacoazul01 commented Dec 12, 2023

Everything except this disabled color is ok?

@QuncCccccc
Copy link
Contributor

QuncCccccc commented Dec 12, 2023

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 search_anchor.dart. That's where we put all these numbers:)

@Macacoazul01
Copy link
Contributor Author

Macacoazul01 commented Dec 14, 2023

@QuncCccccc added the opacity and the test. Can you review, please?

QuncCccccc
QuncCccccc reviewed Dec 19, 2023
View reviewed changes
justinmc
justinmc approved these changes Jan 8, 2024
View reviewed changes
Copy link
Contributor

@justinmc justinmc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with nits. Thanks for the PR!

packages/flutter/lib/src/material/search_anchor.dart
textInputAction: widget.textInputAction,
keyboardType: widget.keyboardType,
child: Opacity(
opacity: widget.enabled ? 1 : _kDisableSearchBarOpacity,
Copy link
Contributor

@justinmc justinmc Jan 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Super nit: 1.0 instead of 1.

@QuncCccccc
Copy link
Contributor

QuncCccccc commented Jan 10, 2024

@Macacoazul01 Could you rebase master to trigger the tests again? I don't think the test failures are related to your change.

@Macacoazul01
Copy link
Contributor Author

Macacoazul01 commented Jan 11, 2024

Finaly

@QuncCccccc QuncCccccc added the autosubmit Merge PR when tree becomes green via auto submit App label Jan 11, 2024
@auto-submit auto-submit bot merged commit e281c39 into flutter:master Jan 11, 2024
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jan 12, 2024
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 12, 2024
@engine-flutter-autoroll
e281c39 Expose 'enable' property to allow the user to disable the Sea…
c2f02ac 
…rchBar (flutter/flutter#137388)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jan 12, 2024
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 12, 2024
@engine-flutter-autoroll
e281c39 Expose 'enable' property to allow the user to disable the Sea…
e58ddf5 
…rchBar (flutter/flutter#137388)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jan 12, 2024
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 12, 2024
@engine-flutter-autoroll
e281c39 Expose 'enable' property to allow the user to disable the Sea…
e46064f 
…rchBar (flutter/flutter#137388)
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Jan 12, 2024
@engine-flutter-autoroll
Roll Flutter from 9f2e681 to 7dc856a (15 revisions) (#5882)
c9d8fb6 
flutter/flutter@9f2e681...7dc856a

2024-01-12 [email protected] Revert "Reverts "Run iOS staging tests with Xcode 15.2"" (flutter/flutter#141420)
2024-01-12 [email protected] Roll Packages from 0744fe6 to d74d687 (5 revisions) (flutter/flutter#141449)
2024-01-12 [email protected] Fix `FlexibleSpaceBar` centered title position and title color (flutter/flutter#140883)
2024-01-12 [email protected] Do not reset framework checkout before running customer tests (flutter/flutter#141013)
2024-01-12 [email protected] Increase delay for checking integration_ui_keyboard_resize test success (flutter/flutter#141301)
2024-01-12 [email protected] Add osx_sdk context for mac builds. (flutter/flutter#141422)
2024-01-12 [email protected] Roll Flutter Engine from ecdaed76f284 to 44a0a6ee4d39 (18 revisions) (flutter/flutter#141432)
2024-01-12 [email protected] Add support for Gradle Kotlin DSL (flutter/flutter#140744)
2024-01-12 [email protected] 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 [email protected] Run iOS staging tests with Xcode 15.2 (flutter/flutter#141392)
2024-01-11 [email protected] Fix `ListWheelScrollView` in an `AnimatedContainer` with zero height throw an error (flutter/flutter#141372)
2024-01-11 [email protected] make asset_test.dart tests not dependent on context (flutter/flutter#141331)
2024-01-11 [email protected] Expose 'enable' property to allow the user to disable the SearchBar (flutter/flutter#137388)
2024-01-11 [email protected] 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 [email protected],[email protected],[email protected] 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
@Macacoazul01 Macacoazul01 deleted the search-anchor branch January 18, 2024 00:38
@Macacoazul01 Macacoazul01 restored the search-anchor branch January 18, 2024 00:38
@Macacoazul01 Macacoazul01 deleted the search-anchor branch January 18, 2024 00:38
@HansMuller HansMuller mentioned this pull request Jan 19, 2024
Closed
@HansMuller HansMuller mentioned this pull request Feb 1, 2024
Closed
2 tasks
@Macacoazul01
Copy link
Contributor Author

Macacoazul01 commented Mar 30, 2024

@HansMuller will this land on the next stable?

@HansMuller
Copy link
Contributor

HansMuller commented Apr 1, 2024

Yes, it should be part of the next stable release.

@darshankawar darshankawar mentioned this pull request Jun 17, 2024
Closed
@Macacoazul01 Macacoazul01 mentioned this pull request Aug 12, 2024
Merged
9 tasks
auto-submit bot pushed a commit that referenced this pull request Sep 4, 2024
@Macacoazul01
added enabled to search anchor (#153256)
ce15e3b 
Added the same parameter from #137388 to the `SearchAnchor`

This PR will fix: #150331
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@QuncCccccc QuncCccccc QuncCccccc approved these changes

@justinmc justinmc justinmc approved these changes

Assignees

No one assigned

Labels

autosubmit Merge PR when tree becomes green via auto submit App f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Macacoazul01 @QuncCccccc @HansMuller @justinmc