Skip to content
This repository was archived by the owner on Jan 13, 2025. It is now read-only.

feat(menu): Added new API to manually set focus when menu is opened #4468

Merged
merged 11 commits into from
Mar 18, 2019

Conversation

abhiomkar
Copy link
Collaborator

@abhiomkar abhiomkar commented Mar 1, 2019

Fixes #3922

Superseded by changes in PR #4587

BREAKING CHANGE: Focus is no more set to first menu item when menu is opened. Introduced new API (setDefaultFocusItemIndex()) to set focus on specific menu item every time the menu is opened. Also introduced new foundation & adapter methods to incorporate this change. Please use setDefaultFocusItemIndex(0) method before menu open to retain previous behaviour.

@acdvorak
Copy link
Contributor

acdvorak commented Mar 1, 2019

I think the BREAKING CHANGE line needs to go at the end of the commit message and PR description, otherwise Lerna won't see it during the release. E.g., see #3337.

Don't forget to update the commit message before you hit "Squash and merge" 😀

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Copy link
Collaborator Author

@abhiomkar abhiomkar 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 the quick review!

@codecov-io
Copy link

codecov-io commented Mar 4, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@ef16a66). Click here to learn what that means.
The diff coverage is 97.56%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #4468   +/-   ##
=========================================
  Coverage          ?   99.02%           
=========================================
  Files             ?      130           
  Lines             ?     6277           
  Branches          ?      820           
=========================================
  Hits              ?     6216           
  Misses            ?       60           
  Partials          ?        1
Impacted Files Coverage Δ
packages/mdc-menu/foundation.ts 96.7% <100%> (ø)
packages/mdc-menu/component.ts 100% <100%> (ø)
packages/mdc-select/component.ts 98.07% <80%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ef16a66...3ee582f. Read the comment docs.

@abhiomkar
Copy link
Collaborator Author

Ping.

Copy link
Contributor

@kfranqueiro kfranqueiro left a comment

Choose a reason for hiding this comment

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

When I test the baseline demo page, for some reason pressing space on the button does nothing, but I'd expect it to act the same as clicking with the mouse. It also seems to lose focus when I do that?

Copy link
Collaborator Author

@abhiomkar abhiomkar left a comment

Choose a reason for hiding this comment

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

Updated menu/fixture.js to handle space key in screenshot test demo. Thanks for spotting it out.

Copy link
Contributor

@kfranqueiro kfranqueiro left a comment

Choose a reason for hiding this comment

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

In the BREAKING CHANGE: note, include info on the call required to match the old default behavior.

@abhiomkar abhiomkar dismissed acdvorak’s stale review March 18, 2019 22:21

Andy's review comments are resolved

@abhiomkar abhiomkar merged commit 42ae5c3 into master Mar 18, 2019
@abhiomkar abhiomkar deleted the feat/menu_focus_api_master branch March 18, 2019 22:41
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MDCMenu: Parameterize whether menu automatically focuses the first item
6 participants