Skip to content
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

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
Contributor

@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" 😀

packages/mdc-menu/adapter.ts Outdated Show resolved Hide resolved
packages/mdc-menu/adapter.ts Outdated Show resolved Hide resolved
packages/mdc-menu/adapter.ts Show resolved Hide resolved
packages/mdc-menu/adapter.ts Outdated Show resolved Hide resolved
packages/mdc-menu/component.ts Outdated Show resolved Hide resolved
packages/mdc-menu/foundation.ts Outdated Show resolved Hide resolved
packages/mdc-menu/foundation.ts Show resolved Hide resolved
packages/mdc-menu/foundation.ts Show resolved Hide resolved
packages/mdc-menu/foundation.ts Outdated Show resolved Hide resolved
packages/mdc-menu/foundation.ts Show resolved Hide resolved
Copy link
Contributor 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!

packages/mdc-menu/foundation.ts Outdated Show resolved Hide resolved
packages/mdc-menu/foundation.ts Show resolved Hide resolved
@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
Contributor 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?

packages/mdc-menu/README.md Outdated Show resolved Hide resolved
packages/mdc-menu/README.md Outdated Show resolved Hide resolved
packages/mdc-menu/adapter.ts Outdated Show resolved Hide resolved
packages/mdc-select/component.ts Outdated Show resolved Hide resolved
test/unit/mdc-menu/mdc-menu.test.js Outdated Show resolved Hide resolved
test/unit/mdc-menu/menu.foundation.test.js Outdated Show resolved Hide resolved
test/unit/mdc-menu/menu.foundation.test.js Outdated Show resolved Hide resolved
Copy link
Contributor 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.

packages/mdc-menu/README.md Outdated Show resolved Hide resolved
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 join this conversation on GitHub. Already have an account? Sign in to comment
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