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): add adapter #3233

Merged
merged 13 commits into from
Jul 30, 2018
Merged

fix(select): add adapter #3233

merged 13 commits into from
Jul 30, 2018

Conversation

moog16
Copy link
Contributor

@moog16 moog16 commented Jul 26, 2018

No description provided.

@mdc-web-bot
Copy link
Collaborator

All 128 screenshot tests passed for commit e63a2ed vs. master! 💯🎉

Matt Goo added 2 commits July 26, 2018 10:06
@codecov-io
Copy link

codecov-io commented Jul 26, 2018

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #3233   +/-   ##
=========================================
  Coverage          ?   98.36%           
=========================================
  Files             ?      120           
  Lines             ?     5130           
  Branches          ?      639           
=========================================
  Hits              ?     5046           
  Misses            ?       84           
  Partials          ?        0
Impacted Files Coverage Δ
packages/mdc-select/constants.js 100% <ø> (ø)
packages/mdc-select/foundation.js 100% <100%> (ø)
packages/mdc-select/index.js 91.08% <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 6ea3600...e31bc21. Read the comment docs.

@mdc-web-bot
Copy link
Collaborator

All 128 screenshot tests passed for commit 974641d vs. master! 💯🎉

@mdc-web-bot
Copy link
Collaborator

All 128 screenshot tests passed for commit cc624d2 vs. master! 💯🎉

@@ -38,6 +38,7 @@
*
* @record
*/

Copy link
Contributor

Choose a reason for hiding this comment

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

How'd this get into this PR? Is this fixing anything?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm weird...don't remember that

| `getValue() => string` | Returns the value selected on the `select` element. |
| `hasClass(className: string) => boolean` | Returns true if the root element has the className in its classList. |
| `hasLabel() => boolean` | Returns true if the `select` has a label associated with it. |
| `hasOutlined() => boolean` | Returns true if the `select` has the notched outline element. |
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo: hasOutlined -> hasOutline

| `activateBottomLine() => void` | Activates the bottom line component. |
| `addClass(className: string) => void` | Adds a class to the root element. |
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we have typically kept correlated APIs together, so can we move removeClass and hasClass back here, and put APIs for the label / outline each together, etc?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

gotcha - i alpha sorted

addClass(className) {}

/**
* Removes a class from the root Element.
Copy link
Contributor

Choose a reason for hiding this comment

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

Element -> element to be consistent with other docs

hasLabel() {}

/**
* Only implement if label exists.
Copy link
Contributor

Choose a reason for hiding this comment

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

I would rephrase all of these "Only implement" comments, e.g. "Returns width of label in pixels, if the label exists."

The adapter APIs are always implemented, but ones like this simply return early if the element in question doesn't exist.

@@ -64,18 +83,27 @@ export default class MDCSelectFoundation extends MDCFoundation {
}
}

/**
* Handles value event changes.
Copy link
Contributor

Choose a reason for hiding this comment

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

Handles value changes, via change event or programmatic updates.

@mdc-web-bot
Copy link
Collaborator

All 128 screenshot tests passed for commit 9967eb8 vs. master! 💯🎉

@mdc-web-bot
Copy link
Collaborator

All 128 screenshot tests passed for commit 9dc3f0b vs. master! 💯🎉

@mdc-web-bot
Copy link
Collaborator

All 128 screenshot tests passed for commit 59d39df vs. master! 💯🎉

@mdc-web-bot
Copy link
Collaborator

All 140 screenshot tests passed for commit 3200c8e vs. master! 💯🎉

@mdc-web-bot
Copy link
Collaborator

All 140 screenshot tests passed for commit e31bc21 vs. master! 💯🎉

@moog16 moog16 merged commit 43b3ac1 into master Jul 30, 2018
@moog16 moog16 deleted the fix/select/add-adapter branch July 30, 2018 22:09
YuoMamoru pushed a commit to YuoMamoru/material-components-web that referenced this pull request Aug 14, 2018
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.

4 participants