-
Notifications
You must be signed in to change notification settings - Fork 879
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.
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
Make NTP setting menu keyboard accessible #2814
Conversation
d7300fe
to
aeda067
Compare
e6ddb38
to
fed8672
Compare
cbdff98
to
8733203
Compare
@imptrx still wip? |
yep - currently waiting on the status of a recently filed issue: brave/brave-browser#5199 to be confirmed since they touch upon the same logic |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comments left
de1d113
to
c6d835f
Compare
cacc638
to
32ac9c8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
code looks good, feature works, and mentioned issues are confirmed to be fixed by these changes. left two questions and a request.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
++
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing, this is now nice and clean.
A good practice would be to make the commits that we end up putting in master work towards keeping the code and code-history understandable. Two things would improve that:
- Make sure each commit message is descriptive about a feature
- always referencing New Tab Page in this case
- Squash commits
- Code that we don't need and never wanted to merge usually just adds noise to the story of the file
- Something like "Address feedback" isn't a meaningful commit message to see why a change was made. If there's no meaningful reason to have it as a commit, then it should be folded in to the feature that is being modified.
In general a PR should have as few commits as possible. If there are multiple commits then they would probably be for very separate features that happen to be in the same PR, which should be a rare thing (prefer multiple PRs).
If you need help learning some good techniques for how to rebase and squash, I'd be happy to share some tips.
In this case maybe there would be a few commits in an ideal world, after looking at your end result:
- New Tab Page: settings tabindex matches visual order
- New Tab Page: settings menu is keyboard accessible
- New Tab Page: settings open / close state is not persisted
- Storybook: Add normalizer for parity with browser webui
… menu is keyboard accessible; closes brave/brave-browser#5041; closes /brave/brave-browser/issues/5199
8db1958
to
8935550
Compare
fixes brave/brave-browser#5041
fixes brave/brave-browser#5199
Brave-ui PR: brave/brave-ui#506Quick View:
Submitter Checklist:
npm run lint
)git rebase master
(if needed).git rebase -i
to squash commits (if needed).Test Plan 1:
space
orenter
will open the setting menuspace
orenter
or 'escape' will allow you to close the menuTest Plan 2:
Reviewer Checklist:
After-merge Checklist:
changes has landed on.