feat(menu): Add --selected class to menu items #2084
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2084 +/- ##
==========================================
- Coverage 99.46% 99.31% -0.16%
==========================================
Files 84 84
Lines 3722 3770 +48
Branches 480 496 +16
==========================================
+ Hits 3702 3744 +42
- Misses 20 26 +6
Continue to review full report at Codecov.
|
packages/mdc-menu/adapter.js
Outdated
| @@ -141,6 +141,34 @@ class MDCMenuAdapter { | |||
|
|
|||
| /** @param {string} height */ | |||
| setMaxHeight(height) {} | |||
|
|
|||
| /** @param {number} index */ | |||
| getOptionAtIndex(index) {} | |||
There was a problem hiding this comment.
does this return something? It is a getter after all...
| @@ -22,6 +22,7 @@ const cssClasses = { | |||
| ANIMATING_OPEN: 'mdc-menu--animating-open', | |||
| ANIMATING_CLOSED: 'mdc-menu--animating-closed', | |||
| LIST_ITEM: 'mdc-list-item', | |||
| SELECTED_LIST_ITEM: 'mdc-list-item--selected', | |||
There was a problem hiding this comment.
you should be able to reference the constants on MDCListFoundation.strings
There was a problem hiding this comment.
MDCListFoundation doesn't exist
packages/mdc-menu/foundation.js
Outdated
| /** @private {boolean} */ | ||
| this.rememberSelection_ = false; | ||
| /** @private {boolean} */ | ||
| this.keyDownWithinMenu_ = false; |
There was a problem hiding this comment.
this keeps track of whether the user currently has a keydown interaction happening on the mdc-menu component?
These sorts of fields can be vague...so add a comment about exactly what it is this field represents
packages/mdc-menu/foundation.js
Outdated
| /** | ||
| * @param {?number} focusIndex | ||
| * @private | ||
| */ | ||
| focusOnOpen_(focusIndex) { | ||
| if (focusIndex === null) { | ||
| // First, try focusing the menu. | ||
| // First focus the last selected item if we're remembering selections and it has been set |
There was a problem hiding this comment.
flip this
"If this instance of MDCMenu remembers selections, and the user has made a selection, then focus the last selected item"
packages/mdc-menu/foundation.js
Outdated
| @@ -241,6 +263,9 @@ class MDCMenuFoundation extends MDCFoundation { | |||
| const isArrowUp = key === 'ArrowUp' || keyCode === 38; | |||
| const isArrowDown = key === 'ArrowDown' || keyCode === 40; | |||
| const isSpace = key === 'Space' || keyCode === 32; | |||
| const isEnter = key === 'Enter' || keyCode === 13; | |||
|
|
|||
| this.keyDownWithinMenu_ = isEnter || isSpace; | |||
There was a problem hiding this comment.
explain whats going on here
packages/mdc-menu/foundation.js
Outdated
| this.adapter_.focusItemAtIndex(this.selectedIndex_); | ||
| return; | ||
| } | ||
| // Then, try focusing the menu. |
There was a problem hiding this comment.
You probably dont need this comment.
packages/mdc-menu/foundation.js
Outdated
| @@ -297,7 +322,12 @@ class MDCMenuFoundation extends MDCFoundation { | |||
| const isEscape = key === 'Escape' || keyCode === 27; | |||
|
|
|||
| if (isEnter || isSpace) { | |||
| this.handlePossibleSelected_(evt); | |||
| // If our menu didn't catch the keydown, then the user likely hit the | |||
There was a problem hiding this comment.
There is no "we" or "our" in comments. The program is the thing running. Try and be specific about which part of the program is doing what action.
For example,
"If MDCMenu does not see a keydown event, then it assumes the user triggered an enter or space event"
Also, how is enter and space NOT a keydown event??
packages/mdc-menu/foundation.js
Outdated
| @@ -564,6 +597,38 @@ class MDCMenuFoundation extends MDCFoundation { | |||
| isOpen() { | |||
| return this.isOpen_; | |||
| } | |||
|
|
|||
| /** @return {?Element} */ | |||
There was a problem hiding this comment.
I think we try NOT to reference HTML Element in our Foundation classes. Maybe this public method is not necessary...
Change as per material-components-web v0.30.0 reference: material-components/material-components-web#2084 Adds `mdc-list-item--selected` class to menu items when `rememberSelection` is set to `true`. #### New Public methods - [x] Add `setRememberSelection(rememberSelection: boolean)` - [x] Add `getSelectedIndex(): number` - [x] Add `setSelectedIndex(index): void` - [x] Add `setMaxHeight(height: number): void` Closes #618
fixes: #2031
related: #536
Adds
mdc-list-item--selectedclass to menu items whenrememberSelectionis set totrue.Auto-focuses on the last selected item when opened when
rememberSelectionis set totrue.