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

fix(list): Change default class to activated #3388

Merged
merged 7 commits into from
Aug 22, 2018

Conversation

williamernest
Copy link
Contributor

fixes: #3383, fixes: #3361

This changes the default class to add/remove from --selected to --activated. It preserves the ability to use --selected by adding a foundation variable useSelectedClass_ (and setter methods).

Removes the ability to toggle the activated class off by clicking the same element twice.

@mdc-web-bot
Copy link
Collaborator

All 353 screenshot tests passed for commit 9c536e2 vs. master! 💯🎉

@codecov-io
Copy link

codecov-io commented Aug 20, 2018

Codecov Report

Merging #3388 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3388      +/-   ##
==========================================
+ Coverage   98.45%   98.45%   +<.01%     
==========================================
  Files         123      123              
  Lines        5181     5185       +4     
  Branches      639      640       +1     
==========================================
+ Hits         5101     5105       +4     
  Misses         80       80
Impacted Files Coverage Δ
packages/mdc-list/constants.js 100% <ø> (ø) ⬆️
packages/mdc-list/index.js 98.85% <100%> (+0.07%) ⬆️
packages/mdc-list/foundation.js 100% <100%> (ø) ⬆️

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 2cf8487...7ffb214. Read the comment docs.

@mdc-web-bot
Copy link
Collaborator

All 353 screenshot tests passed for commit a7e7adc vs. master! 💯🎉

@williamernest williamernest force-pushed the fix/list/allow-activated-or-selected-class branch from a7e7adc to 06121ac Compare August 21, 2018 00:05
@mdc-web-bot
Copy link
Collaborator

All 353 screenshot tests passed for commit 06121ac vs. master! 💯🎉

@mdc-web-bot
Copy link
Collaborator

All 353 screenshot tests passed for commit cb31cb9 vs. master! 💯🎉

@mdc-web-bot
Copy link
Collaborator

All 353 screenshot tests passed for commit f1f0b5d vs. master! 💯🎉

@mdc-web-bot
Copy link
Collaborator

All 353 screenshot tests passed for commit 8b1a968 vs. master! 💯🎉

@@ -134,7 +134,7 @@ OR
### Single Selection List

MDC List can handle selecting/deselecting list elements based on click or keyboard action. When enabled, the `space` and `enter` keys (or `click` event) will trigger an
Copy link
Contributor

Choose a reason for hiding this comment

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

selecting/deselecting

is this still relevant?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, when clicked it will deselect the previously selected list item and select the currently selected list item.

`setSelectedIndex()` should be called with the initially selected element immediately after the foundation is
instantiated.
the `aria-selected` attribute, the `mdc-list-item--selected` or `mdc-list-item--activated` class should be added,
and the `tabindex` of the selected element should be `0`. The first list item should have the `tabindex` updated
Copy link
Contributor

Choose a reason for hiding this comment

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

is it mandator for user to set tabindex -1 explicitly on first element? Doesn't list automatically sets tabindex to -1 to all other list items?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the user has marked an element as selected, they should render the first list element to tabindex=-1. The foundation may not be instantiated immediately when the page is loaded (such as in Wiz).

this.foundation_.setUseActivatedClass(true);
}

this.singleSelection = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

is it safe to assume user wants to have single selection mode when list has selected item?

Multi-selectable list can initially have one item selected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it is safe to assume that the user has a single selection list when using the selected or activated classes. There isn't a multi-selectable list, but this can be changed if we implement one.

demos/list.html Outdated
@@ -53,7 +53,7 @@
info
</a>
</li>
<li class="mdc-list-item">
<li class="mdc-list-item mdc-list-item--activated">
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be helpful to have separate demos for following cases:

  • singleSelect: with --selected
  • singleSelect: with --activated
  • multiSelect

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add a demo for --activated.

Copy link
Contributor

@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.

LGTM.

demos/list.html Outdated
@@ -53,7 +53,7 @@
info
</a>
</li>
<li class="mdc-list-item">
<li class="mdc-list-item mdc-list-item--activated">
Copy link
Contributor

Choose a reason for hiding this comment

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

aria-selected and tabindex attr are missing for activated item.

@mdc-web-bot
Copy link
Collaborator

All 353 screenshot tests passed for commit 7082f39 vs. master! 💯🎉

@mdc-web-bot
Copy link
Collaborator

All 353 screenshot tests passed for commit 7ffb214 vs. master! 💯🎉

@williamernest williamernest merged commit 5590412 into master Aug 22, 2018
@williamernest williamernest deleted the fix/list/allow-activated-or-selected-class branch August 22, 2018 02:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants