feat(menu): add setSelectedIndex to set selected item in menu selection group #4620

moog16 · 2019-04-17T18:53:44Z

BREAKING CHANGE: Replaced adapter methods getParentElement, getSelectedElementIndex with getSelectedSiblingOfItemAtIndex, isSelectableItemAtIndex.

packages/mdc-menu/foundation.ts

abhiomkar

Looks good overall. Added few more comments.

packages/mdc-menu/foundation.ts

packages/mdc-menu/component.ts

packages/mdc-menu/foundation.ts

abhiomkar · 2019-04-18T22:33:04Z

test/screenshot/spec/mdc-menu/classes/multiple-menu-selection-group.html

@@ -0,0 +1,165 @@
+<!DOCTYPE html>
+<!--
+  Copyright 2018 Google Inc.


packages/mdc-menu/README.md

packages/mdc-menu/adapter.ts

packages/mdc-menu/component.ts

packages/mdc-menu/foundation.ts

codecov-io · 2019-04-23T00:19:00Z

Codecov Report

❗ No coverage uploaded for pull request base (develop@6de4115). Click here to learn what that means.
The diff coverage is 96.29%.

@@            Coverage Diff             @@
##             develop    #4620   +/-   ##
==========================================
  Coverage           ?   98.95%           
==========================================
  Files              ?      129           
  Lines              ?     6297           
  Branches           ?      820           
==========================================
  Hits               ?     6231           
  Misses             ?       65           
  Partials           ?        1

Impacted Files	Coverage Δ
packages/mdc-menu/component.ts	`100% <100%> (ø)`
packages/mdc-menu/foundation.ts	`96.25% <94.73%> (ø)`

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 6de4115...b0de834. Read the comment docs.

…material-components/material-components-web into feat/menu/selected-item-selection-group t

mdc-web-bot · 2019-04-23T23:52:03Z

🤖 Beep boop!

Screenshot test report 🚦

3 screenshots changed from master on commit d2ea93d:

https://storage.googleapis.com/mdc-web-screenshot-tests/travis/2019/04/23/23_49_05_735/report/report.html?utm_source=github&utm_medium=pr_comment

Details

3 Added:

spec/mdc-menu/classes/multiple-menu-selection-group.html

packages/mdc-menu/foundation.ts

mdc-web-bot · 2019-04-26T21:55:09Z

🤖 Beep boop!

Screenshot test report 🚦

3 screenshots changed from master on commit d83778e:

https://storage.googleapis.com/mdc-web-screenshot-tests/travis/2019/04/26/21_52_06_604/report/report.html?utm_source=github&utm_medium=pr_comment

Details

3 Added:

spec/mdc-menu/classes/multiple-menu-selection-group.html

abhiomkar

LGTM 🎉

Left few more comments mostly minor.

abhiomkar · 2019-04-27T23:17:17Z

packages/mdc-dialog/package-lock.json

  "requires": true,
+  "lockfileVersion": 1,


Revert all package-lock.json changes

packages/mdc-menu/README.md

abhiomkar · 2019-04-27T23:28:02Z

packages/mdc-menu/foundation.ts

-      this.adapter_.removeAttributeFromElementAtIndex(selectedIndex, strings.ARIA_SELECTED_ATTR);
-      this.adapter_.removeClassFromElementAtIndex(selectedIndex, cssClasses.MENU_SELECTED_LIST_ITEM);
+  setSelectedIndex(index: number) {
+    if (!this.isIndexInRange_(index)) {


Since this function is validating the index and throws error when index is invalid, let's call this validateIndex_. The return statement will never be executed with current structure.

this.isIndexValid_(index);

sorry i'm confused. Do you want to call it isIndexValid_ or validateIndex_?

I'm suggesting, this.validateIndex_().

As per current implementation isIndexValid_() does not always return boolean.

Ah sorry I was confused...current implementation is isIndexInRange_. I'll change to validateIndex and return boolean.

Sorry for not being clearer. isIndexInRange_ seems perfectly fine if the function is just returning a boolean without any side-effects (i.e., throwing an error).

My earlier comment was that this if condition will never fail, because the method throws JS error instead of returning false value. (I noticed this in coverage report).

OH - I understood what you meant after I implemented this. I think I changed it correctly. I just need to update tests.

test/screenshot/spec/mdc-menu/classes/menu-selection-group.html

test/screenshot/spec/mdc-menu/classes/multiple-menu-selection-group.html

test/unit/mdc-menu/mdc-menu.test.js

test/unit/mdc-menu/menu.foundation.test.js

abhiomkar · 2019-04-27T23:49:31Z

test/unit/mdc-menu/menu.foundation.test.js

-    {times: 0});
-  td.verify(mockAdapter.addClassToElementAtIndex(0, cssClasses.MENU_SELECTED_LIST_ITEM), {times: 1});
+  try {
+    foundation.setSelectedIndex(5);


You'll need to use assertThrow here.

abhiomkar

Also, updated PR description with BREAKING CHANGE tag, make sure to add this line in your commit message when you hit on that Squash & Merge button. :)

mdc-web-bot · 2019-04-29T19:23:25Z

🤖 Beep boop!

Screenshot test report 🚦

3 screenshots changed from master on commit b828770:

https://storage.googleapis.com/mdc-web-screenshot-tests/travis/2019/04/29/19_20_24_890/report/report.html?utm_source=github&utm_medium=pr_comment

Details

3 Added:

spec/mdc-menu/classes/multiple-menu-selection-group.html

mdc-web-bot · 2019-04-29T19:28:07Z

🤖 Beep boop!

Screenshot test report 🚦

3 screenshots changed from master on commit 03bb86e:

https://storage.googleapis.com/mdc-web-screenshot-tests/travis/2019/04/29/19_25_03_566/report/report.html?utm_source=github&utm_medium=pr_comment

Details

3 Added:

spec/mdc-menu/classes/multiple-menu-selection-group.html

moog16 · 2019-05-06T15:42:00Z

This Pr requires #4679 to be merged first

mdc-web-bot · 2019-05-06T18:30:04Z

🤖 Beep boop!

Screenshot test report 🚦

3 screenshots changed from develop on commit b0de834:

https://storage.googleapis.com/mdc-web-screenshot-tests/travis/2019/05/06/18_27_30_683/report/report.html?utm_source=github&utm_medium=pr_comment

Details

3 Added:

spec/mdc-menu/classes/multiple-menu-selection-group.html

…on group (#4620) BREAKING CHANGE: Replaced adapter methods getParentElement, getSelectedElementIndex with getSelectedSiblingOfItemAtIndex, isSelectableItemAtIndex.

feat(menu): add setSelectionGroup

4cd5a13

moog16 assigned abhiomkar Apr 17, 2019

googlebot added the cla: yes label Apr 17, 2019

Matt Goo added 2 commits April 17, 2019 11:59

docs: add readme update

6a0daaa

fix: lint

9ae65b5

abhiomkar requested changes Apr 18, 2019

View reviewed changes

packages/mdc-menu/foundation.ts Outdated Show resolved Hide resolved

packages/mdc-menu/foundation.ts Outdated Show resolved Hide resolved

packages/mdc-menu/foundation.ts Outdated Show resolved Hide resolved

packages/mdc-menu/foundation.ts Outdated Show resolved Hide resolved

Matt Goo added 3 commits April 18, 2019 10:37

fix: pr changes

56c48cb

fix: update docs

1629476

fix: select item by index

5eff199

abhiomkar requested changes Apr 18, 2019

View reviewed changes

Matt Goo added 2 commits April 19, 2019 16:17

fix: PR changes

f5d478e

Merge branch 'master' into feat/menu/selected-item-selection-group

1b86312

abhiomkar requested changes Apr 22, 2019

View reviewed changes

moog16 mentioned this pull request Apr 23, 2019

menu should implement a getSelectedIndex method #4634

Open

fix: setSelectedIndex

2d95bb0

Matt Goo added 4 commits April 23, 2019 14:16

fix: isINdexinRange

fb9ddd0

Merge branch 'master' into feat/menu/selected-item-selection-group

b5eb1fa

fix: add golden

4c09a15

Merge branch 'feat/menu/selected-item-selection-group' of github.com:…

d2ea93d

…material-components/material-components-web into feat/menu/selected-item-selection-group t

fix(list): Cleanup techdebt to avoid referencing Element in foundation

0ce0a80

abhiomkar reviewed Apr 24, 2019

View reviewed changes

packages/mdc-menu/foundation.ts Outdated Show resolved Hide resolved

fix: tests and readme3

d83778e

abhiomkar approved these changes Apr 27, 2019

View reviewed changes

abhiomkar reviewed Apr 27, 2019

View reviewed changes

Matt Goo added 2 commits April 29, 2019 12:04

fix: pr changes

b828770

fix: change to validateIndex_

03bb86e

moog16 changed the base branch from master to develop April 29, 2019 19:12

fix: readme

b9a0392

abhiomkar changed the title ~~feat(menu): add setSelectedIndex~~ feat(menu): add setSelectedIndex to set selected item in menu selection group May 2, 2019

Matt Goo added 5 commits May 2, 2019 12:32

fix: lint

31293d6

Merge branch 'master' into feat/menu/selected-item-selection-group

dc750e0

fix: update packagelocks

5afe9bd

Merge branch 'develop' into feat/menu/selected-item-selection-group

f9b78a5

fix: revert locks

7d854be

Merge branch 'develop' into feat/menu/selected-item-selection-group

b0de834

moog16 merged commit a031927 into develop May 6, 2019

moog16 deleted the feat/menu/selected-item-selection-group branch May 6, 2019 18:33

moog16 mentioned this pull request May 6, 2019

MDC Menu: Implement API to change selected item in selection group #4525

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(menu): add setSelectedIndex to set selected item in menu selection group #4620

feat(menu): add setSelectedIndex to set selected item in menu selection group #4620

moog16 commented Apr 17, 2019 •

edited by abhiomkar

Loading

abhiomkar left a comment

abhiomkar Apr 18, 2019

codecov-io commented Apr 23, 2019 •

edited

Loading

mdc-web-bot commented Apr 23, 2019

3 Added:

mdc-web-bot commented Apr 26, 2019

3 Added:

abhiomkar left a comment

abhiomkar Apr 27, 2019

abhiomkar Apr 27, 2019

moog16 Apr 29, 2019

abhiomkar Apr 29, 2019

moog16 Apr 29, 2019

abhiomkar May 1, 2019

moog16 May 1, 2019

abhiomkar Apr 27, 2019

abhiomkar left a comment

mdc-web-bot commented Apr 29, 2019

3 Added:

mdc-web-bot commented Apr 29, 2019

3 Added:

moog16 commented May 6, 2019

mdc-web-bot commented May 6, 2019

3 Added:

feat(menu): add setSelectedIndex to set selected item in menu selection group #4620

feat(menu): add setSelectedIndex to set selected item in menu selection group #4620

Conversation

moog16 commented Apr 17, 2019 • edited by abhiomkar Loading

abhiomkar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Apr 23, 2019 • edited Loading

Codecov Report

mdc-web-bot commented Apr 23, 2019

Screenshot test report 🚦

3 Added:

mdc-web-bot commented Apr 26, 2019

Screenshot test report 🚦

3 Added:

abhiomkar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abhiomkar left a comment

Choose a reason for hiding this comment

mdc-web-bot commented Apr 29, 2019

Screenshot test report 🚦

3 Added:

mdc-web-bot commented Apr 29, 2019

Screenshot test report 🚦

3 Added:

moog16 commented May 6, 2019

mdc-web-bot commented May 6, 2019

Screenshot test report 🚦

3 Added:

moog16 commented Apr 17, 2019 •

edited by abhiomkar

Loading

codecov-io commented Apr 23, 2019 •

edited

Loading