Skip to content

Configurable padding around FocusNodes in Scrollables#96815

Merged
fluttergithubbot merged 7 commits intoflutter:masterfrom
ds84182:ensure_visible
Apr 6, 2022
Merged

Configurable padding around FocusNodes in Scrollables#96815
fluttergithubbot merged 7 commits intoflutter:masterfrom
ds84182:ensure_visible

Conversation

@ds84182
Copy link
Contributor

@ds84182 ds84182 commented Jan 18, 2022

Fixes #96813

If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.

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: focus Focus traversal, gaining or losing focus f: scrolling Viewports, list views, slivers, etc. framework flutter/packages/flutter repository. See also f: labels. labels Jan 18, 2022
@goderbauer
Copy link
Member

goderbauer commented Jan 20, 2022

@ds84182 Can you fix the analyzer issue?

gspencergoog
gspencergoog previously requested changes Jan 20, 2022
View reviewed changes
@Piinks Piinks added the c: new feature Nothing broken; request for a new capability label Jan 21, 2022
Piinks
Piinks reviewed Jan 21, 2022
View reviewed changes
@goderbauer
Copy link
Member

goderbauer commented Feb 9, 2022

(Triage) @ds84182 Do you still have plans to come back to this one and address the feedback given above?

@ds84182
Copy link
Contributor Author

ds84182 commented Feb 10, 2022

Yes, I've been out for the last two weeks but I'll be able to come back to this on Monday

@skia-gold
Copy link

skia-gold commented Feb 24, 2022

Gold has detected about 3 new digest(s) on patchset 5.
View them at https://flutter-gold.skia.org/cl/github/96815

@skia-gold
Copy link

skia-gold commented Feb 24, 2022

Gold has detected about 2 new digest(s) on patchset 5.
View them at https://flutter-gold.skia.org/cl/github/96815

Piinks
Piinks reviewed Feb 25, 2022
View reviewed changes
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.

Hi @ds84182, is this PR ready for another review? Or are you still working on it?

@ds84182
Copy link
Contributor Author

ds84182 commented Feb 28, 2022

Still working on it, have one last test to finish

@goderbauer
Copy link
Member

goderbauer commented Mar 16, 2022

(triage) @ds84182 Is this one still on your radar?

@ds84182
Copy link
Contributor Author

ds84182 commented Mar 16, 2022

Yes, sorry this fell of my radar. I can add the final test today

@skia-gold
Copy link

skia-gold commented Mar 16, 2022

Gold has detected about 32 new digest(s) on patchset 6.
View them at https://flutter-gold.skia.org/cl/github/96815

@Piinks
Copy link
Contributor

Piinks commented Mar 22, 2022

It looks like this will need to be rebased with master in order to pass these checks. Thanks again for working on this, apologies for the review delay. :)

@ds84182
Copy link
Contributor Author

ds84182 commented Mar 23, 2022

Rebased, will address the comment for scrollRevealPadding's name in a moment

@ds84182 ds84182 requested a review from gspencergoog March 23, 2022 20:53
Piinks
Piinks approved these changes Mar 28, 2022
View reviewed changes
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.

Flutter_LGTM

@Piinks
Copy link
Contributor

Piinks commented Mar 31, 2022

Apologies for the churn @ds84182, when you have a chance can you update your branch with the latest from master? The G testing shard is stuck here.

@fluttergithubbot
Copy link
Contributor

fluttergithubbot commented Apr 4, 2022

This pull request is not suitable for automatic merging in its current state.

  • The status or check suite Google testing has failed. Please fix the issues identified (or deflake) before re-applying this label.

@fluttergithubbot fluttergithubbot merged commit 81baee4 into flutter:master Apr 6, 2022
This was referenced Apr 6, 2022
Merged
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Apr 6, 2022
@engine-flutter-autoroll
81baee4 Configurable padding around FocusNodes in Scrollables (flutte…
0163771 
…r/flutter#96815)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 6, 2022
@engine-flutter-autoroll
81baee4 Configurable padding around FocusNodes in Scrollables (flutte…
bc5cee0 
…r/flutter#96815)
Piinks added a commit that referenced this pull request Apr 12, 2022
@Piinks
Revert "Configurable padding around FocusNodes in Scrollables (#96815)"
7591c37 
This reverts commit 81baee4.
@Piinks Piinks mentioned this pull request Apr 12, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Piinks Piinks Piinks approved these changes

@gspencergoog gspencergoog Awaiting requested review from gspencergoog

Assignees

No one assigned

Labels

c: new feature Nothing broken; request for a new capability f: focus Focus traversal, gaining or losing focus f: scrolling Viewports, list views, slivers, etc. 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.

Allow focus traversal to leave additional padding when an item is focused within a Scrollable widget

6 participants

@ds84182 @goderbauer @skia-gold @Piinks @fluttergithubbot @gspencergoog