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 #2125

Conversation

dpraul
Copy link
Contributor

@dpraul dpraul commented Jan 30, 2018

Closes #1810

@codecov-io
Copy link

codecov-io commented Jan 30, 2018

Codecov Report

Merging #2125 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2125      +/-   ##
==========================================
- Coverage   99.14%   99.14%   -0.01%     
==========================================
  Files          90       90              
  Lines        3843     3842       -1     
  Branches      497      496       -1     
==========================================
- Hits         3810     3809       -1     
  Misses         33       33
Impacted Files Coverage Δ
packages/mdc-select/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 b82c203...350f47c. Read the comment docs.

@@ -129,6 +129,13 @@ style dependencies for both the mdc-list and mdc-menu for this component to func
</div>
```

#### Preventing [FOUC](https://en.wikipedia.org/wiki/Flash_of_unstyled_content)
Copy link
Contributor

Choose a reason for hiding this comment

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

This section should be removed. It doesn't really address the FOUC issue. It seems like this section duplicates the Select with pre-selected option section example, and they should be merged under that section.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, looks like I fat-fingered the markdown here - meant to nest the Preventing FOUC section under the Select with pre-selected option.

Should I fix the nesting so that it provides context to the Select with pre-selected option section, or just remove the Preventing FOUC section altogether?

Copy link
Contributor

Choose a reason for hiding this comment

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

Change it to

When dealing with the select component that has pre-selected values, you'll want to ensure that you
render `mdc-select__label` with the `mdc-select__label--float-above` modifier class and the selected option with `aria-selected`. This will
ensure that the label moves out of the way of the select's value and prevents a Flash Of
Un-styled Content (**FOUC**).

so that it more closely matches the text field explanation for pre-filled values. Also, it should be above the example on Line 99.

Copy link
Contributor

@williamernest williamernest left a comment

Choose a reason for hiding this comment

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

One minor fix, but then I think it's ready to go!

@@ -129,6 +129,13 @@ style dependencies for both the mdc-list and mdc-menu for this component to func
</div>
```

#### Preventing [FOUC](https://en.wikipedia.org/wiki/Flash_of_unstyled_content)
Copy link
Contributor

Choose a reason for hiding this comment

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

Change it to

When dealing with the select component that has pre-selected values, you'll want to ensure that you
render `mdc-select__label` with the `mdc-select__label--float-above` modifier class and the selected option with `aria-selected`. This will
ensure that the label moves out of the way of the select's value and prevents a Flash Of
Un-styled Content (**FOUC**).

so that it more closely matches the text field explanation for pre-filled values. Also, it should be above the example on Line 99.

Copy link
Contributor

@williamernest williamernest left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for doing this!

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.

None yet

3 participants