-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
feat(chips): Baseline chip and chip set #2083
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2083 +/- ##
==========================================
- Coverage 99.25% 99.13% -0.12%
==========================================
Files 84 90 +6
Lines 3772 3831 +59
Branches 489 491 +2
==========================================
+ Hits 3744 3798 +54
- Misses 28 33 +5
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
About half-done with review but figured I'd send comments now
package-lock.json
Outdated
@@ -1074,7 +1074,7 @@ | |||
"base64-js": { | |||
"version": "1.2.1", | |||
"resolved": "https://registry.npmjs.org/base64-js/-/base64-js-1.2.1.tgz", | |||
"integrity": "sha512-dwVUVIXsBZXwTuwnXI9RK8sBmgq09NDHzyR9SAph9eqk76gKK2JSQmZARC2zRC81JC2QTtxD0ARU5qTS25gIGw==", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
package-lock shouldn't be in this diff.
On this branch, git checkout master -- package-lock.json
demos/index.html
Outdated
<span class="demo-catalog-list-icon mdc-list-item__graphic"><img src="/images/ic_chips_24dp.svg" /></span> | ||
<span class="mdc-list-item__text"> | ||
Chips | ||
<span class="mdc-list-item__secondary-text">Chips for actions, selection, and input </span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove trailing space
packages/mdc-chips/package-lock.json
Outdated
@@ -0,0 +1,37 @@ | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... I'm curious how this ended up generated, since I don't see package-lock files in any other of our own packages
folders? I suspect it's unnecessary.
demos/chips.html
Outdated
<script> | ||
demoReady(function() { | ||
var chipSets = document.querySelectorAll( | ||
'.mdc-chip-set:not([data-demo-no-auto-js])' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we planning to show css-only chips? Otherwise the :not
here isn't doing anything right now.
Also, this statement can fit on one line.
demos/chips.html
Outdated
var chipSets = document.querySelectorAll( | ||
'.mdc-chip-set:not([data-demo-no-auto-js])' | ||
); | ||
for (var i = 0, chipSet; chipSet = chipSets[i]; i++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[].forEach.call(chipSets, function(chipSet) {
packages/mdc-chips/chip/index.js
Outdated
} | ||
|
||
initialize() { | ||
this.ripple_ = new MDCRipple(this.root_); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question... Do you think we need a layout
method, like text field has to re-layout the ripple? It would presumably be mostly relevant to situations where a chip is initialized inside something that is initially hidden...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might... I can't think of any use cases right now but we can see if it's necessary as we build out more features on chips.
packages/mdc-chips/README.md
Outdated
|
||
# Chips | ||
|
||
Chips are compact elements that represent an attribute, text, entity, or action. They allow users to enter information, select a choice, filter content, or trigger an action. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First sentence sounds kind of confusing to me (an element that "represents" text??). I'd consider condensing to just "Chips are compact elements that allow users to enter information, select a choice, filter content, or trigger an action."
packages/mdc-chips/README.md
Outdated
|
||
The MDC Chips module is comprised of two JavaScript classes: | ||
* `MDCChip` defines the behavior of a single chip | ||
* `MDCChipSet` defines the behavior of a chip within a specific set. For example, chips in an entry chip set behave differently from those in a filter chip set. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"a chip" -> "chips"?
packages/mdc-chips/README.md
Outdated
--- | --- | ||
`registerInteractionHandler(evtType: string, handler: EventListener) => void` | Registers an event listener on the root element | ||
`deregisterInteractionHandler(evtType: string, handler: EventListener) => void` | Deregisters an event listener on the root element | ||
`notifyInteraction() => void` | Emits a custom event "MDCChip:interaction" denoting the chip has been interacted with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mention that this event bubbles, like we did with text field icon
packages/mdc-chips/chip/adapter.js
Outdated
* Returns the text content in the chip. | ||
* @return {string} | ||
*/ | ||
getText() {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is intended to eventually support e.g. selection APIs, I'd be a bit concerned of the consequences of i18n (i.e. the returned text would vary by language)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah this is a good point. I'll remove it for now and wait until we have the actual selection API built to determine the best way for querying chips.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed rest of JS outside of tests, will get to that next.
/** | ||
* @param {!MDCChipSetAdapter=} adapter | ||
*/ | ||
constructor(adapter = /** @type {!MDCChipSetAdapter} */ ({})) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't realize we started doing this in some of our newer foundations, but I don't think the default parameter value here makes sense, since errors will occur whether the adapter is empty or undefined.
packages/mdc-chips/README.md
Outdated
--- | --- | ||
`registerInteractionHandler(evtType: string, handler: EventListener) => void` | Registers an event listener on the root element | ||
`deregisterInteractionHandler(evtType: string, handler: EventListener) => void` | Deregisters an event listener on the root element | ||
`notifyInteraction() => void` | Emits a custom event "MDCChip:interaction" denoting the chip has been interacted with, which bubbles to the top-level text field element |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"text field element"? 🤔 replace with chip set?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops, thanks for catching!
packages/mdc-chips/chip-set/index.js
Outdated
}))); | ||
} | ||
|
||
instantiateChips_(chipFactory) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need type information to compile with closure?
Also, this PR doesn't seem to be adding mdc-chips
to closureWhitelist
in package.json... should we be doing that from the outset with this package?
packages/mdc-chips/chip/constants.js
Outdated
/** @enum {string} */ | ||
const strings = { | ||
INTERACTION_EVENT: 'MDCChip:interaction', | ||
TEXT_SELECTOR: '.mdc-chip__text', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is unused now; should it be removed here?
|
||
.mdc-chip__text { | ||
display: inline-block; | ||
vertical-align: middle; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI I noticed something funny with this style... with it, chips are 33px tall, but without it, they're 32px tall. (Only checked in Chrome so far.)
Not sure if we're aiming for a specific height for chips, or if this is moot pending the addition of icons anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting... the mdc-chip__text
div is 18px but for some reason the mdc-chip
content becomes 19px.
From the experimental/chip
branch it seems that both mdc-chip
content and mdc-chip__icon
height are 24px once icons are added (which is the expected result).
Not sure what's causing this, but a fix for now would be to make the mdc-chip__text
content 17px since that won't affect anything that comes afterwards. Any thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My first question would be... are chips w/o icons expected to be less tall than chips w/ icons? This would be weird if there's any situation in which you can have a mix of some with icons and some without within the same set, but OTOH if they're all expected to eventually match the with-icons size then this point may become moot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Chips w/o icons are expected to be the same height as chips with icons.
But chips with icons are also 32px, which means that we need to fix this for chips w/o icons anyways.
foundation.destroy(); | ||
|
||
td.verify(mockAdapter.deregisterInteractionHandler('click', td.matchers.isA(Function))); | ||
td.verify(mockAdapter.deregisterInteractionHandler('click', td.matchers.isA(Function))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this supposed to be keydown
?
Added a few extra chip set tests to check that the child chips are being properly instantiated/destroyed. I'm getting weird errors when trying to add the module to the Closure whitelist, so I'm still working on that. |
Added |
hasClass(className) {} | ||
} | ||
|
||
export {MDCChipSetAdapter}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed you had to update your imports due to this exporting an object containing the adapter rather than just exporting the adapter directly. I'm pretty sure we generally don't export an object unless we need to expose other things in addition (e.g. with text field), so I think you can remove the curly braces in the chip and chip-set adapters as well as their respective imports.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
MDC Chips is a new package in MDC Web for the chips component. It is comprised of two parts: the chip itself and the set that the chip belongs to.
This PR includes:
mdc-chip
,mdc-chip-set
, andmdc-chip__text
classesMDCChip
andMDCChipSet
MDCChip
andMDCChipSet
This PR doesn't include:
Note: MDC Chips has a similar model to MDC Tabs, which is comprised of multiple mandatory components, as opposed to MDC Text Field, which is a single main component with multiple optional components. Thus there is a single README for MDC Chips.
Resolves #2003