Skip to content

Conversation

@adarsh-technocrat
Copy link
Contributor

@adarsh-technocrat adarsh-technocrat commented Dec 18, 2020

Hello, @redbrogdon @AyushBherwani1998 👋🏼 it would be fantastic if you could review my very first PR here in the Flutter samples Repository.
The PR replaces the custom search field used by Veggieseasons sample app with CupertinoSearchTextField

fixes: #618

child: CupertinoSearchTextField(
controller: controller,
focusNode: focusNode,
decoration: null,
Copy link
Member

Choose a reason for hiding this comment

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

It's by default a nullable type, so you can avoid passing null.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure @AyushBherwani1998 I'll do the necessary changes and revert you back

@AyushBherwani1998
Copy link
Member

AyushBherwani1998 commented Jan 5, 2021

Hey @adarsh-technocrat, thanks for putting a PR for this. Sorry for the delays, was on a break.

The idea is to completely remove the search_bar.dart. After using CupertinoSearchTextField the file has redundant code which is to provide the backdrop filter to the search bar and using a Row. CupertinoSearchTextField, by default has a backdrop filter, so in my opinion you can directly use it instead of SearchBar in search.dart.

@adarsh-technocrat
Copy link
Contributor Author

Hey @adarsh-technocrat, thanks for putting a PR for this. Sorry for the delays, was on a break.

The idea is to completely remove the search_bar.dart. After using CupertinoSearchTextField the file has redundant code which is to provide the backdrop filter to the search bar and using a Row. CupertinoSearchTextField, by default has a backdrop filter, so in my opinion you can directly use it instead of SearchBar in search.dart.

Okay, @AyushBherwani1998 I'll do the changes

@adarsh-technocrat
Copy link
Contributor Author

Hello, @AyushBherwani1998 is this fine to keep CupertinoSearchTextField with the default backdrop filter here because CupertinoSearchTextField hardly seems to be visible when we scroll the list or either we can provide background color with some opacity or add a background container to it because CupertinoSearchTextField does not come with an option to increase or decrease the CupertinoSearchTextField opacity level. What's your thought on this?

CupertinoSearchTextField with default backdrop filter

CupertinoSearchTextField with backgroundcolor and opacity

@AyushBherwani1998
Copy link
Member

Try to use Styles.searchBackground(themeData) which was used before for the background color, and see if it works.

because CupertinoSearchTextField does not come with an option to increase or decrease the CupertinoSearchTextField opacity level

If you want to tweak the opacity, you can simply use .withOpacity to change the opacity.

@adarsh-technocrat
Copy link
Contributor Author

Try to use Styles.searchBackground(themeData) which was used before for the background color, and see if it works.

because CupertinoSearchTextField does not come with an option to increase or decrease the CupertinoSearchTextField opacity level

If you want to tweak the opacity, you can simply use .withOpacity to change the opacity.

Hey @AyushBherwani1998 I have done the changes in the PR it works very well as suggested by you just a request please review my PR once again.

Copy link
Contributor

@redbrogdon redbrogdon 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 submitting this change. It's going to remove more lines of code than it adds, which is my favorite kind of PR.

@adarsh-technocrat adarsh-technocrat force-pushed the updated-custom-search-field branch 2 times, most recently from 3fe605c to 544d5f8 Compare January 9, 2021 14:12
@adarsh-technocrat adarsh-technocrat force-pushed the updated-custom-search-field branch from 544d5f8 to 9a9fee7 Compare January 9, 2021 15:27
@redbrogdon redbrogdon merged commit d16d35e into flutter:master Jan 31, 2021
@redbrogdon
Copy link
Contributor

Thanks, @adarsh-technocrat! I'm glad to see the new widget in place.

@adarsh-technocrat
Copy link
Contributor Author

Thanks, @adarsh-technocrat! I'm glad to see the new widget in place.

Thank you @redbrogdon and @AyushBherwani1998! as this was my first PR that merge into master though it was just a small change looking forward to learn and happy to contribute more to the repo :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Update Veggieseasons to use CupertinoSearchTextField

3 participants