Skip to content

Conversation

@TahaTesser
Copy link
Member

@TahaTesser TahaTesser commented Dec 21, 2022

fixes #117420

Description

Refactored the code to get the exact label height.
This is required when label sizes change or when a custom label style font size is provided or when the window is resized.

Also added golden tests as well.

M2

Screen.Recording.2022-12-21.at.18.21.38.mov

M3

Screen.Recording.2022-12-21.at.18.21.56.mov

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.

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

@flutter-dashboard flutter-dashboard bot added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Dec 21, 2022
@TahaTesser

This comment was marked as resolved.

@flutter-dashboard

This comment was marked as resolved.

@flutter-dashboard flutter-dashboard bot added the will affect goldens Changes to golden files label Dec 22, 2022
@flutter-dashboard

This comment was marked as resolved.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fixing this up a frame late doesn't seem like a good solution. If the InkWell will not need the icon's position until after layout has happened then couldn't we look up the icon's position via a GlobalKey?

Copy link
Member Author

@TahaTesser TahaTesser Jan 16, 2023

Choose a reason for hiding this comment

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

Thanks for the suggestion. I was hesitant to use a GlobalKey but given the requirement here, I think it's worth it.

Updating the PR with GlobalKey to check for label size, as it is different between M2 and M3.

@TahaTesser TahaTesser force-pushed the nav_bar_indicator_label branch from 3a7d1d6 to 839fb92 Compare January 17, 2023 12:25
@TahaTesser TahaTesser force-pushed the nav_bar_indicator_label branch from 839fb92 to 8fe9f3f Compare January 18, 2023 09:04
@flutter-dashboard
Copy link

Golden file changes are available for triage from new commit, Click here to view.

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.

Changes reported for pull request #117473 at sha 8fe9f3f

@TahaTesser
Copy link
Member Author

@HansMuller
Updated the PR based on the suggestion. Can you please take a look?

@TahaTesser TahaTesser requested a review from HansMuller January 18, 2023 15:37
Copy link
Contributor

@HansMuller HansMuller left a comment

Choose a reason for hiding this comment

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

LGTM

pathMatcher: isPathThat(
includes: <Offset>[
// Left center.
Offset(indicatorCenter.dx - (includedIndicatorSize.width / 2), indicatorCenter.dy),
Copy link
Contributor

Choose a reason for hiding this comment

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

I usually wouldn't recommend factoring a test however in this case it might be worth factoring out a local utility method that generates a list of the four center offsets given an indicator. If you think that would be worthwhile it would be OK to land this PR as-is and then factor these tests a little as a follow-on PR.

@override
RectCallback? getRectCallback(RenderBox referenceBox) {
final RenderBox labelBox = labelKey.currentContext!.findRenderObject()! as RenderBox;
final Rect labelRect = labelBox.localToGlobal(Offset.zero) & labelBox.size;
Copy link
Contributor

Choose a reason for hiding this comment

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

NICE

@TahaTesser TahaTesser changed the title Refactor NavigationBar ink rectCallback to use position of the rendered icon & add golden tests Refactor NavigationBar's rectCallback to account for label height & add golden tests Jan 19, 2023
Copy link
Contributor

@Piinks Piinks left a comment

Choose a reason for hiding this comment

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

Golden images are approved, LGTM!

@Piinks Piinks added the autosubmit Merge PR when tree becomes green via auto submit App label Jan 20, 2023
@auto-submit auto-submit bot merged commit 74645b4 into flutter:master Jan 20, 2023
@TahaTesser TahaTesser deleted the nav_bar_indicator_label branch January 20, 2023 08:51
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Jan 20, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 20, 2023
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Jan 20, 2023
* 030288d Revert "[macos] add flavor options to commands in the `flutter_tool` (#118421)" (flutter/flutter#118858)

* 9acf34d Roll Flutter Engine from 26b6609c603b to 7d40e77d0035 (2 revisions) (flutter/flutter#118852)

* ec51d32 Remove unnecessary null checks in ‘dev/conductor’ (flutter/flutter#118843)

* 54217bd Remove unnecessary null checks in `dev/benchmarks` (flutter/flutter#118840)

* 98c18ca Remove unnecessary null checks in examples/ (flutter/flutter#118848)

* 99b5262 Remove unnecessary null checks in dev/tools (flutter/flutter#118845)

* 52d1205 Roll Flutter Engine from 7d40e77d0035 to 730e88fb6787 (3 revisions) (flutter/flutter#118869)

* ee9c9b6 3876320cb Roll Skia from aedfc8695954 to 1b3aa8b6e1cc (43 revisions) (flutter/engine#39024) (flutter/flutter#118871)

* 589f2eb d2436a536 Extract WideToUTF16String/UTF16StringToWide to FML (flutter/engine#39020) (flutter/flutter#118873)

* 74645b4 Fix `NavigationBar` indicator ripple doesn't account for label height (flutter/flutter#117473)

* f78b1f3 dfe67f4c7 Roll Skia from 1b3aa8b6e1cc to f6a5c806294d (11 revisions) (flutter/engine#39027) (flutter/flutter#118874)

* 572f0a1 66e177a3d Roll Dart SDK from ddf70a598f27 to fbbfc122dba6 (9 revisions) (flutter/engine#39029) (flutter/flutter#118878)

* 26472b5 ccccee513 [macos] Synthesize modifier keys events on pointer events (flutter/engine#37870) (flutter/flutter#118880)

* 095b1ab Checkbox borderSide lerp bug fix (flutter/flutter#118728)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Jan 20, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Jan 20, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Jan 20, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Jan 20, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Jan 20, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Jan 20, 2023
auto-submit bot pushed a commit to flutter/plugins that referenced this pull request Jan 20, 2023
* 3348987 Add new macos target configured for flavors (flutter/flutter#117352)

* 6277520 Roll Plugins from 4e5cf2d to 11361d0 (4 revisions) (flutter/flutter#118682)

* 997d436 Fix applyBoxFit's handling of fitWidth and fitHeight. (flutter/flutter#117185)

* 8a58ec5 Roll Flutter Engine from f79030440948 to c52b290813bd (29 revisions) (flutter/flutter#118720)

* 374f09e [flutter_tools] No more implicit --no-sound-null-safety (flutter/flutter#118491)

* ae1cc18 remove single-view assumption from `paintImage` (flutter/flutter#118721)

* bb8b96a Fix path for require.js (flutter/flutter#118722)

* c83a698 update uikit view documentation (flutter/flutter#118715)

* 2b3ca0d Bump github/codeql-action from 2.1.38 to 2.1.39 (flutter/flutter#118735)

* 666dccc [macOS] bringup new e2e_summary devicelab test (flutter/flutter#118717)

* d07b88e Docs fix an=>a (flutter/flutter#118652)

* 11d21e0 Add @pragma('vm:entry-point') to RestorableRouteBuilder arguments (flutter/flutter#118738)

* 7d9eaab Appbar iconTheme override fix (flutter/flutter#118681)

* 6f70830 Roll Flutter Engine from c52b290813bd to 290636c1cb6b (2 revisions) (flutter/flutter#118743)

* b3059d2 Bump activesupport from 6.1.5 to 6.1.7.1 in /dev/ci/mac (flutter/flutter#118745)

* ffcf63a Add verbose flag to plugin_dependencies_test to debug flake (flutter/flutter#118755)

* 2609212 2a11023c7 [ios_platform_view] more precision when determine if a clip rrect is necessary (flutter/engine#38965) (flutter/flutter#118751)

* 21fb443 8ed6790b5 Bump chrome_and_driver version to 110. (flutter/engine#38986) (flutter/flutter#118758)

* e5c9d06 Forgot to remove emulator flag. (flutter/flutter#118762)

* 6a9b2db 95b0c151f Roll Dart SDK from 645fd748e79e to ddf70a598f27 (14 revisions) (flutter/engine#38990) (flutter/flutter#118763)

* 0bbb5ec 40f7f0f09 Roll Fuchsia Mac SDK from P5QcCJU8I71xVXuMT... to tlYMsnCv86Fjt5LfF... (flutter/engine#38994) (flutter/flutter#118771)

* d53cc4a [macOS] New e2e_summary benchmark fails without Cocoapods. (flutter/flutter#118754)

* 3e71e0c Updated `ListTile` documentation, add Material 3 example and other `ListTile` examples fixes. (flutter/flutter#118705)

* 213b3cb Check whether slider is mounted before interaction, no-op if unmounted (flutter/flutter#113556)

* 06909cc Update packages + fix tests for javascript mime change (flutter/flutter#118617)

* 46c7fd1 88e61d8bd Remove references to Observatory (flutter/engine#38919) (flutter/flutter#118793)

* b9ab640 Remove incorrect statement in documentation (flutter/flutter#118636)

* ea36b3a Add focus detector to CupertinoSwitch (flutter/flutter#118345)

* 9b5ea30 Switching over from iOS-15 to iOS-16 in .ci.yaml. (flutter/flutter#118807)

* 67ffaef 29a0582a1 Roll Fuchsia Mac SDK from tlYMsnCv86Fjt5LfF... to 6oiZwMyNsjucSxTHJ... (flutter/engine#39004) (flutter/flutter#118817)

* 5cd2d4c Support iOS wireless debugging (flutter/flutter#118104)

* cbf2e16 Revert "Support iOS wireless debugging (#118104)" (flutter/flutter#118826)

* 2258590 Do not run Mac_arm64_ios run_debug_test_macos in presubmit during iPhone 11 migration (flutter/flutter#118828)

* 1dd7f45 Add `build macos --config-only` option. (flutter/flutter#118649)

* 22520f5 [macOS] Add timeline summary benchmarks (flutter/flutter#118748)

* 99e4ca5 Roll Flutter Engine from 29a0582a1d5f to 78bbea005d27 (2 revisions) (flutter/flutter#118829)

* c5ceff1 [flutter_tools] Ensure service worker starts caching assets since first load  (flutter/flutter#116833)

* 818bb4e Roll Flutter Engine from 78bbea005d27 to 26b6609c603b (3 revisions) (flutter/flutter#118839)

* 09bd0f6 Support logging 'flutter run' communication to DAP clients (flutter/flutter#118674)

* 73096fd [macos] add flavor options to commands in the `flutter_tool` (flutter/flutter#118421)

* 030288d Revert "[macos] add flavor options to commands in the `flutter_tool` (#118421)" (flutter/flutter#118858)

* 9acf34d Roll Flutter Engine from 26b6609c603b to 7d40e77d0035 (2 revisions) (flutter/flutter#118852)

* ec51d32 Remove unnecessary null checks in ‘dev/conductor’ (flutter/flutter#118843)

* 54217bd Remove unnecessary null checks in `dev/benchmarks` (flutter/flutter#118840)

* 98c18ca Remove unnecessary null checks in examples/ (flutter/flutter#118848)

* 99b5262 Remove unnecessary null checks in dev/tools (flutter/flutter#118845)

* 52d1205 Roll Flutter Engine from 7d40e77d0035 to 730e88fb6787 (3 revisions) (flutter/flutter#118869)

* ee9c9b6 3876320cb Roll Skia from aedfc8695954 to 1b3aa8b6e1cc (43 revisions) (flutter/engine#39024) (flutter/flutter#118871)

* 589f2eb d2436a536 Extract WideToUTF16String/UTF16StringToWide to FML (flutter/engine#39020) (flutter/flutter#118873)

* 74645b4 Fix `NavigationBar` indicator ripple doesn't account for label height (flutter/flutter#117473)

* f78b1f3 dfe67f4c7 Roll Skia from 1b3aa8b6e1cc to f6a5c806294d (11 revisions) (flutter/engine#39027) (flutter/flutter#118874)

* 572f0a1 66e177a3d Roll Dart SDK from ddf70a598f27 to fbbfc122dba6 (9 revisions) (flutter/engine#39029) (flutter/flutter#118878)

* 26472b5 ccccee513 [macos] Synthesize modifier keys events on pointer events (flutter/engine#37870) (flutter/flutter#118880)

* 095b1ab Checkbox borderSide lerp bug fix (flutter/flutter#118728)

* ec6ff90 Roll Flutter Engine from ccccee513fb2 to d84b3dc74c9f (2 revisions) (flutter/flutter#118893)

* 492d572 Cleanup obsolete --compact-async compiler option (flutter/flutter#118894)

* f291eb3 Remove unnecessary null checks in integration_test (flutter/flutter#118861)

* ab3c822 Remove unnecessary null checks in dev/devicelab (flutter/flutter#118842)

* bf72f5e 58eb1061e Revert "Remove references to Observatory (#38919)" (flutter/engine#39035) (flutter/flutter#118899)

* a07e8a6 [reland] Support wireless debugging (flutter/flutter#118895)
mauricioluz pushed a commit to mauricioluz/plugins that referenced this pull request Jan 26, 2023
* 3348987 Add new macos target configured for flavors (flutter/flutter#117352)

* 6277520 Roll Plugins from 4e5cf2d to 11361d0 (4 revisions) (flutter/flutter#118682)

* 997d436 Fix applyBoxFit's handling of fitWidth and fitHeight. (flutter/flutter#117185)

* 8a58ec5 Roll Flutter Engine from f79030440948 to c52b290813bd (29 revisions) (flutter/flutter#118720)

* 374f09e [flutter_tools] No more implicit --no-sound-null-safety (flutter/flutter#118491)

* ae1cc18 remove single-view assumption from `paintImage` (flutter/flutter#118721)

* bb8b96a Fix path for require.js (flutter/flutter#118722)

* c83a698 update uikit view documentation (flutter/flutter#118715)

* 2b3ca0d Bump github/codeql-action from 2.1.38 to 2.1.39 (flutter/flutter#118735)

* 666dccc [macOS] bringup new e2e_summary devicelab test (flutter/flutter#118717)

* d07b88e Docs fix an=>a (flutter/flutter#118652)

* 11d21e0 Add @pragma('vm:entry-point') to RestorableRouteBuilder arguments (flutter/flutter#118738)

* 7d9eaab Appbar iconTheme override fix (flutter/flutter#118681)

* 6f70830 Roll Flutter Engine from c52b290813bd to 290636c1cb6b (2 revisions) (flutter/flutter#118743)

* b3059d2 Bump activesupport from 6.1.5 to 6.1.7.1 in /dev/ci/mac (flutter/flutter#118745)

* ffcf63a Add verbose flag to plugin_dependencies_test to debug flake (flutter/flutter#118755)

* 2609212 2a11023c7 [ios_platform_view] more precision when determine if a clip rrect is necessary (flutter/engine#38965) (flutter/flutter#118751)

* 21fb443 8ed6790b5 Bump chrome_and_driver version to 110. (flutter/engine#38986) (flutter/flutter#118758)

* e5c9d06 Forgot to remove emulator flag. (flutter/flutter#118762)

* 6a9b2db 95b0c151f Roll Dart SDK from 645fd748e79e to ddf70a598f27 (14 revisions) (flutter/engine#38990) (flutter/flutter#118763)

* 0bbb5ec 40f7f0f09 Roll Fuchsia Mac SDK from P5QcCJU8I71xVXuMT... to tlYMsnCv86Fjt5LfF... (flutter/engine#38994) (flutter/flutter#118771)

* d53cc4a [macOS] New e2e_summary benchmark fails without Cocoapods. (flutter/flutter#118754)

* 3e71e0c Updated `ListTile` documentation, add Material 3 example and other `ListTile` examples fixes. (flutter/flutter#118705)

* 213b3cb Check whether slider is mounted before interaction, no-op if unmounted (flutter/flutter#113556)

* 06909cc Update packages + fix tests for javascript mime change (flutter/flutter#118617)

* 46c7fd1 88e61d8bd Remove references to Observatory (flutter/engine#38919) (flutter/flutter#118793)

* b9ab640 Remove incorrect statement in documentation (flutter/flutter#118636)

* ea36b3a Add focus detector to CupertinoSwitch (flutter/flutter#118345)

* 9b5ea30 Switching over from iOS-15 to iOS-16 in .ci.yaml. (flutter/flutter#118807)

* 67ffaef 29a0582a1 Roll Fuchsia Mac SDK from tlYMsnCv86Fjt5LfF... to 6oiZwMyNsjucSxTHJ... (flutter/engine#39004) (flutter/flutter#118817)

* 5cd2d4c Support iOS wireless debugging (flutter/flutter#118104)

* cbf2e16 Revert "Support iOS wireless debugging (#118104)" (flutter/flutter#118826)

* 2258590 Do not run Mac_arm64_ios run_debug_test_macos in presubmit during iPhone 11 migration (flutter/flutter#118828)

* 1dd7f45 Add `build macos --config-only` option. (flutter/flutter#118649)

* 22520f5 [macOS] Add timeline summary benchmarks (flutter/flutter#118748)

* 99e4ca5 Roll Flutter Engine from 29a0582a1d5f to 78bbea005d27 (2 revisions) (flutter/flutter#118829)

* c5ceff1 [flutter_tools] Ensure service worker starts caching assets since first load  (flutter/flutter#116833)

* 818bb4e Roll Flutter Engine from 78bbea005d27 to 26b6609c603b (3 revisions) (flutter/flutter#118839)

* 09bd0f6 Support logging 'flutter run' communication to DAP clients (flutter/flutter#118674)

* 73096fd [macos] add flavor options to commands in the `flutter_tool` (flutter/flutter#118421)

* 030288d Revert "[macos] add flavor options to commands in the `flutter_tool` (#118421)" (flutter/flutter#118858)

* 9acf34d Roll Flutter Engine from 26b6609c603b to 7d40e77d0035 (2 revisions) (flutter/flutter#118852)

* ec51d32 Remove unnecessary null checks in ‘dev/conductor’ (flutter/flutter#118843)

* 54217bd Remove unnecessary null checks in `dev/benchmarks` (flutter/flutter#118840)

* 98c18ca Remove unnecessary null checks in examples/ (flutter/flutter#118848)

* 99b5262 Remove unnecessary null checks in dev/tools (flutter/flutter#118845)

* 52d1205 Roll Flutter Engine from 7d40e77d0035 to 730e88fb6787 (3 revisions) (flutter/flutter#118869)

* ee9c9b6 3876320cb Roll Skia from aedfc8695954 to 1b3aa8b6e1cc (43 revisions) (flutter/engine#39024) (flutter/flutter#118871)

* 589f2eb d2436a536 Extract WideToUTF16String/UTF16StringToWide to FML (flutter/engine#39020) (flutter/flutter#118873)

* 74645b4 Fix `NavigationBar` indicator ripple doesn't account for label height (flutter/flutter#117473)

* f78b1f3 dfe67f4c7 Roll Skia from 1b3aa8b6e1cc to f6a5c806294d (11 revisions) (flutter/engine#39027) (flutter/flutter#118874)

* 572f0a1 66e177a3d Roll Dart SDK from ddf70a598f27 to fbbfc122dba6 (9 revisions) (flutter/engine#39029) (flutter/flutter#118878)

* 26472b5 ccccee513 [macos] Synthesize modifier keys events on pointer events (flutter/engine#37870) (flutter/flutter#118880)

* 095b1ab Checkbox borderSide lerp bug fix (flutter/flutter#118728)

* ec6ff90 Roll Flutter Engine from ccccee513fb2 to d84b3dc74c9f (2 revisions) (flutter/flutter#118893)

* 492d572 Cleanup obsolete --compact-async compiler option (flutter/flutter#118894)

* f291eb3 Remove unnecessary null checks in integration_test (flutter/flutter#118861)

* ab3c822 Remove unnecessary null checks in dev/devicelab (flutter/flutter#118842)

* bf72f5e 58eb1061e Revert "Remove references to Observatory (#38919)" (flutter/engine#39035) (flutter/flutter#118899)

* a07e8a6 [reland] Support wireless debugging (flutter/flutter#118895)
godofredoc pushed a commit that referenced this pull request Feb 7, 2023
* Fix `NavigationBar` ripple for non-default `NavigationDestinationLabelBehavior` (#116888)

* Fix `NavigationBar` indicator ripple doesn't account for label height (#117473)

* Apply suggestions from code review

Co-authored-by: Michael Goderbauer <[email protected]>

---------

Co-authored-by: Taha Tesser <[email protected]>
Co-authored-by: Xilai Zhang <[email protected]>
Co-authored-by: Michael Goderbauer <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

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. will affect goldens Changes to golden files

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[NavigationBar] ink ripple positioned is shifted in M3

3 participants