Add padding options to `SearchAnchor` #152508

Rexios80 · 2024-07-29T21:13:47Z

Cherry-picked padding changes from #148856

Adds padding configuration options to SearchAnchor. This PR adds the following:

viewBarPadding: The padding for items inside the SearchBar in the SearchAnchor view
viewPadding: The padding to use around the entire SearchAnchor view

Working towards #148852

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.
I followed the breaking change policy and added Data Driven Fixes where supported.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

…chor-padding-config

dev/tools/gen_defaults/lib/search_view_template.dart

…or-padding-config

…ios80/flutter into feature/search-anchor-padding-config

Rexios80 · 2024-08-07T19:16:02Z

@QuncCccccc I made viewBarPadding just EdgeInsets instead of a WidgetStateProperty

Piinks

Two important questions here:

Can a user use the Padding widget to achieve the same effect as viewBarPadding?
Can a user use the Padding widget to achieve the same effect as viewPadding?

If they can, we might not want to complicate the API by adding more ways to aceive the same effect.

Piinks · 2024-08-08T16:58:52Z

packages/flutter/lib/src/material/search_anchor.dart

+  /// If null, the value of [SearchViewThemeData.padding] will be used.
+  final EdgeInsets? viewPadding;


These names should match.

I don't think they should. This field is on SearchAnchor, but that referenced theme object is for the SearchView.

This property can help provide some space between the edge of the view and the edge of the screen for better visuals, so when the screen is small, the view has a way to not touch the screen edge. This is why we want to create this property, right? I think we can add some doc for the use case:)

Piinks · 2024-08-08T16:59:11Z

packages/flutter/lib/src/material/search_anchor.dart


+  /// The padding to use for the search view.
+  ///
+  /// Has no effect if the search view is full-screen.


Why would padding not affect the view by making it smaller?

The padding is disabled if the view is full screen because the search view is supposed to fill the entire screen

I also think maybe we don't need this special handle because we don't really have any specs for this property and its default behavior. Even though I can't think of a concrete use case, I just feel we can still use this property to make the "full-screen" view looks smaller? If users don't want to use it in full-screen mode, they can set the padding to zero, but the current implementation seems always dismiss this property. Just feel a conflict between two property(isFullScreen and this property) is not super ideal:)

If users don't want to use it in full-screen mode, they can set the padding to zero

This would require the users to know if the view wants to make itself fullscreen or not. The view does this based on the platform.

I just feel we can still use this property to make the "full-screen" view looks smaller

I agree. This is a common behavior. For example, the Viewport of a ScrollView will size itself smaller when wrapped with padding.

AFAICT, this part of the PR is what remains to resolve before it lands.

@Piinks

This would require the users to know if the view wants to make itself fullscreen or not. The view does this based on the platform.

How should we deal with this? I only want the padding to apply to the non-fullscreen view, and manually checking the platform to determine if the view will be full screen or not seems pretty clunky.

I only want the padding to apply to the non-fullscreen view

Why though? I would expect the behavior to be the same in either situation. Having special cases baked in makes it harder for the user to understand what is happening and how to get the results they want. :)

Why though?

Because I only added the padding to have the search view not touch the bottom of the window on desktop web browsers.

Having special cases baked in makes it harder for the user to understand what is happening and how to get the results they want. :)

There is already a special case built in to show the full screen view based on the platform... I didn't even know the view did that until it was brought up in PR review, but now that I do know (and see my app behave that way on mobile browsers) I do not want the padding to apply to the full screen view.

Changing the padding if the view is full screen requires internal implementation knowledge of SearchAnchor which is not a good developer UX at all.

Piinks · 2024-08-08T16:59:50Z

packages/flutter/lib/src/material/search_view_theme.dart

+    this.padding,
+    this.barPadding,


These names should definitely match the ones on the widget that they apply to.

They match the existing naming conventions of other properties in this file to the best of my knowledge

Rexios80 · 2024-08-08T17:51:58Z

Two important questions here

Where would these Padding widgets go? I don't see any builders for the search view.

QuncCccccc

Sorry for the late response. Left some comments below. Please let me know if there's any questions:)

QuncCccccc · 2024-08-14T05:31:07Z

packages/flutter/lib/src/material/search_anchor.dart

    MaterialStateProperty<BorderSide?>? barSide,
    MaterialStateProperty<OutlinedBorder?>? barShape,
    MaterialStateProperty<EdgeInsetsGeometry?>? barPadding,
+    EdgeInsets? viewBarPadding,


I think this is a little bit extra because we've already have barPadding above. Currently the barPadding doesn't apply to the bar in the search view, so we probably can remove this parameter and add

viewBarPadding: barPadding?.resolve(<WidgetState>{}),

in _SearchAnchorWithSearchBar constructor so that both search bars are synced.

The default barPadding and viewBarPadding do NOT match, which is why I created this property in the first place. I do not think we can consolidate these.

packages/flutter/lib/src/material/search_anchor.dart

QuncCccccc · 2024-08-14T06:09:28Z

packages/flutter/lib/src/material/search_anchor.dart


+  /// The padding to use for the search view.
+  ///
+  /// Has no effect if the search view is full-screen.


I also think maybe we don't need this special handle because we don't really have any specs for this property and its default behavior. Even though I can't think of a concrete use case, I just feel we can still use this property to make the "full-screen" view looks smaller? If users don't want to use it in full-screen mode, they can set the padding to zero, but the current implementation seems always dismiss this property. Just feel a conflict between two property(isFullScreen and this property) is not super ideal:)

QuncCccccc · 2024-08-14T06:14:32Z

packages/flutter/lib/src/material/search_anchor.dart

+  /// If null, the value of [SearchViewThemeData.padding] will be used.
+  final EdgeInsets? viewPadding;


This property can help provide some space between the edge of the view and the edge of the screen for better visuals, so when the screen is small, the view has a way to not touch the screen edge. This is why we want to create this property, right? I think we can add some doc for the use case:)

Piinks · 2024-08-16T23:11:54Z

Where would these Padding widgets go? I don't see any builders for the search view.

Sounds like the answer to the questions then is no? That's all I wanted to know so that we don't introduce unnecessary API surface. :)

Rexios80 · 2024-09-04T23:38:52Z

@QuncCccccc @Piinks what's the next step here?

Piinks · 2024-09-11T20:39:32Z

packages/flutter/lib/src/material/search_anchor.dart

+                            child: SearchBar(
+                              autoFocus: true,
+                              constraints: headerConstraints ?? (widget.showFullScreenView ? BoxConstraints(minHeight: _SearchViewDefaultsM3.fullScreenBarHeight) : null),
+                              padding: WidgetStatePropertyAll<EdgeInsetsGeometry?>(effectiveBarPadding),


Oh wait I spoke maybe a little too soon, was the prior behavior that SearchBar was not provided any padding? It looks like SearchBar.padding then was by default 16.0 horizontally. Is the change to 8 here breaking?

Does this not make the default 8?

flutter/packages/flutter/lib/src/material/search_anchor.dart

Lines 1590 to 1591 in 50a190c

MaterialStateProperty<EdgeInsetsGeometry>? get padding =>

const MaterialStatePropertyAll<EdgeInsetsGeometry>(EdgeInsets.symmetric(horizontal: 8.0));

Yes, what I was pointing out is that is not the current default. Why is the default changing in this PR?

I know the default padding is different between the base SearchBar and the SearchBar in the SearchView. Let me try and untangle this.

Actually... That code snippet I sent is from the current main branch. So the current default is already 8.

The value of 16 is passed in here:

flutter/packages/flutter/lib/src/material/search_anchor.dart

Line 1086 in 50a190c

padding: barPadding ?? const MaterialStatePropertyAll<EdgeInsets>(EdgeInsets.symmetric(horizontal: 16.0)),

I don't think this behavior has changed

The code is really confusing with this. It definitely tripped me up for a while when I was working on it.

…nfig' into feature/search-anchor-padding-config

…or-padding-config

QuncCccccc

LGTM. Sorry for the late response.

Rexios80 · 2024-10-04T23:53:40Z

@Piinks are you happy with the state of this?

Piinks

I am! Thanks for your patience. A lot of us have been traveling the last month.
LGTM

Rexios80 added 3 commits July 29, 2024 17:03

Cherry-pick padding changes from flutter#148856

9522a0e

Resolve bar padding with theme and defaults

71deea7

Fix name

a4352ef

Rexios80 requested review from Piinks and QuncCccccc July 29, 2024 21:13

github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. labels Jul 29, 2024

Merge remote-tracking branch 'upstream/master' into feature/search-an…

74b5362

…chor-padding-config

Rexios80 commented Jul 29, 2024

View reviewed changes

dev/tools/gen_defaults/lib/search_view_template.dart Outdated Show resolved Hide resolved

Rexios80 added 2 commits July 29, 2024 17:26

Actually don't use viewPadding if full screen

851f543

Update tests

6f4ea3e

Rexios80 marked this pull request as draft July 29, 2024 21:31

Rexios80 marked this pull request as ready for review July 29, 2024 21:32

Rexios80 mentioned this pull request Jul 29, 2024

Add constraint options to SearchAnchor suggestions builder #148856

Merged

9 tasks

Rexios80 added 5 commits July 29, 2024 18:40

Merge branch 'master' into feature/search-anchor-padding-config

11f2a1b

Merge branch 'master' into feature/search-anchor-padding-config

5470635

Merge remote-tracking branch 'upstream/main' into feature/search-anch…

3bd7acf

…or-padding-config

Don't use WidgetStateProperty

4407f08

Merge branch 'feature/search-anchor-padding-config' of github.com:Rex…

0639154

…ios80/flutter into feature/search-anchor-padding-config

Piinks reviewed Aug 8, 2024

View reviewed changes

QuncCccccc reviewed Aug 14, 2024

View reviewed changes

Use EdgeInsetsGeometry instead of EdgeInsets

89b276d

Rexios80 requested a review from QuncCccccc August 16, 2024 17:31

Rexios80 added 4 commits August 26, 2024 12:48

Cherry-pick padding changes from flutter#148856

ec50b7b

Resolve bar padding with theme and defaults

6f6f5b8

Fix name

3a91976

Actually don't use viewPadding if full screen

d90e5a7

Rexios80 added 3 commits August 26, 2024 12:48

Update tests

a83ca22

Don't use WidgetStateProperty

f7bc282

Use EdgeInsetsGeometry instead of EdgeInsets

bbceb28

Piinks force-pushed the feature/search-anchor-padding-config branch from 89b276d to bbceb28 Compare August 26, 2024 17:48

This comment was marked as spam.

Sign in to view

Piinks reviewed Sep 11, 2024

View reviewed changes

Rexios80 added 3 commits September 12, 2024 05:01

Merge remote-tracking branch 'origin/feature/search-anchor-padding-co…

bd45f66

…nfig' into feature/search-anchor-padding-config

Merge branch 'main' into feature/search-anchor-padding-config

9dcefba

Merge remote-tracking branch 'upstream/main' into feature/search-anch…

23c6a8c

…or-padding-config

Rexios80 requested a review from Piinks September 17, 2024 16:47

QuncCccccc approved these changes Oct 4, 2024

View reviewed changes

Piinks approved these changes Oct 15, 2024

View reviewed changes

Piinks added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 15, 2024

auto-submit bot merged commit e96aaab into flutter:master Oct 15, 2024

engine-flutter-autoroll mentioned this pull request Oct 16, 2024

Manual roll Flutter from 6790525ce673 to 5644c032b388 (67 revisions) flutter/packages#7884

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 16, 2024

e96aaab Add padding options to SearchAnchor (flutter/flutter#152508)

cfb3b95

engine-flutter-autoroll mentioned this pull request Oct 17, 2024

Manual roll Flutter from 6790525ce673 to 62414ee44e10 (80 revisions) flutter/packages#7888

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 17, 2024

e96aaab Add padding options to SearchAnchor (flutter/flutter#152508)

7673539

Piinks mentioned this pull request Oct 18, 2024

Flutter Framework Team: Notable Commits #121415

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 12, 2025

e96aaab Add padding options to SearchAnchor (flutter/flutter#152508)

c072ba0

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 13, 2025

e96aaab Add padding options to SearchAnchor (flutter/flutter#152508)

3aa30fe

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 13, 2025

e96aaab Add padding options to SearchAnchor (flutter/flutter#152508)

53fe202

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 6, 2025

e96aaab Add padding options to SearchAnchor (flutter/flutter#152508)

a9592e6

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 7, 2025

e96aaab Add padding options to SearchAnchor (flutter/flutter#152508)

49e043f

		/// If null, the value of [SearchViewThemeData.padding] will be used.
		final EdgeInsets? viewPadding;

	MaterialStateProperty<EdgeInsetsGeometry>? get padding =>
	const MaterialStatePropertyAll<EdgeInsetsGeometry>(EdgeInsets.symmetric(horizontal: 8.0));

Add padding options to SearchAnchor #152508

Add padding options to SearchAnchor #152508

Uh oh!

Conversation

Rexios80 commented Jul 29, 2024

Pre-launch Checklist

Uh oh!

Uh oh!

Rexios80 commented Aug 7, 2024

Uh oh!

Piinks left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Rexios80 Aug 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Rexios80 commented Aug 8, 2024

Uh oh!

QuncCccccc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Piinks commented Aug 16, 2024

Uh oh!

This comment was marked as spam.

Rexios80 commented Sep 4, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

QuncCccccc left a comment

Choose a reason for hiding this comment

Uh oh!

Rexios80 commented Oct 4, 2024

Add padding options to `SearchAnchor` #152508

Add padding options to `SearchAnchor` #152508

Rexios80 Aug 8, 2024 •

edited

Loading