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(select): pre-selected option correctly floats label #2283

Conversation

dpraul
Copy link
Contributor

@dpraul dpraul commented Feb 21, 2018

Closes #2282, fixing regression from #2244

@codecov-io
Copy link

codecov-io commented Feb 21, 2018

Codecov Report

Merging #2283 into master will decrease coverage by 0.25%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2283      +/-   ##
==========================================
- Coverage   99.21%   98.96%   -0.26%     
==========================================
  Files          99       93       -6     
  Lines        3967     3856     -111     
  Branches      510      500      -10     
==========================================
- Hits         3936     3816     -120     
- Misses         31       40       +9
Impacted Files Coverage Δ
packages/mdc-select/foundation.js 100% <100%> (ø) ⬆️
packages/mdc-select/label/index.js 62.5% <0%> (-37.5%) ⬇️
packages/mdc-select/label/foundation.js 71.42% <0%> (-28.58%) ⬇️
packages/mdc-line-ripple/foundation.js 90.32% <0%> (-9.68%) ⬇️
packages/mdc-chips/chip/index.js 66.66% <0%> (-8.34%) ⬇️
packages/mdc-chips/chip/constants.js 100% <0%> (ø) ⬆️
packages/mdc-select/constants.js 100% <0%> (ø) ⬆️
packages/mdc-chips/chip-set/index.js 100% <0%> (ø) ⬆️
packages/mdc-chips/chip-set/constants.js 100% <0%> (ø) ⬆️
packages/mdc-select/index.js 100% <0%> (ø) ⬆️
... and 9 more

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 d0a475a...d07710e. Read the comment docs.

@williamernest williamernest self-assigned this Feb 21, 2018
@williamernest
Copy link
Contributor

williamernest commented Feb 21, 2018

Thanks for submitting this PR. The label should float when the menu is opened, and not only when an item is selected.

@@ -105,9 +105,6 @@ export default class MDCSelectFoundation extends MDCFoundation {
};
this.cancelHandler_ = () => {
this.close_();
if (this.selectedIndex_ === -1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is needed to have the floating label class removed when the menu is closed.

@@ -212,7 +212,6 @@ export default class MDCSelectFoundation extends MDCFoundation {
const focusIndex = this.selectedIndex_ < 0 ? 0 : this.selectedIndex_;

this.setMenuStylesForOpenAtIndex_(focusIndex);
this.adapter_.floatLabel(true);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is needed to ensure the label floats when the menu is opened.

@@ -155,6 +152,9 @@ export default class MDCSelectFoundation extends MDCFoundation {
if (this.selectedIndex_ >= 0) {
selectedTextContent = this.adapter_.getTextForOptionAtIndex(this.selectedIndex_).trim();
this.adapter_.setAttrForOptionAtIndex(this.selectedIndex_, 'aria-selected', 'true');
this.adapter_.floatLabel(true);
} else {
this.adapter_.floatLabel(false);
Copy link
Contributor

Choose a reason for hiding this comment

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

This should only called if the menu is not opened, so that the label is always floating if the menu is opened.

if (this.adapter_.isMenuOpen()) {
    this.adapter_.floatLabel(false);
}

@williamernest
Copy link
Contributor

@dpraul Thanks for submitting this. We got this fixed in #2306 with tests to be sure we don't regress again.

@dpraul dpraul deleted the fix/select/pre-selected-option-regression branch February 27, 2018 20:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Select with pre-selected option doesn't float label (redux)
3 participants