-
Notifications
You must be signed in to change notification settings - Fork 842
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
Add EuiFacetButton #1167
Add EuiFacetButton #1167
Conversation
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.
Looked through it all, learned alot, played around with it locally... it's all great 👍
src-docs/src/views/button/facet.js
Outdated
}, { | ||
id: 'facet5', | ||
label: 'Just here to show truncation of really long labels', | ||
iconColor: VISUALIZATION_COLORS[5], |
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 think it's useful to include quantity
on the truncated example, otherwise it looks like the text pushes the quantity display away.
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.
Makes sense, I'll add
|
||
expect(component) | ||
.toMatchSnapshot(); | ||
}); |
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.
please add tests for the various states, similar to features displayed in the docs (disabled, loading, with color or avatar, etc)
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.
ahh yeah... ha...
children, | ||
className, | ||
icon, | ||
isDisabled, |
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 remember we discussed isFoo
vs. foo
, but don't remember where we ended up?
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 carried this over from the other buttons (specifically empty button). Figured we should at least be consistent there.
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.
Between types of buttons, that is.
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.
makes sense to me!
Can you update the mockup to show some facets in the selected state? Playing with them in the docs I'm not sure the selected state is communicative enough. |
- Updated tests - Allowing `0` as a quantity
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.
LGTM! pulled & played with the docs
thanks for the additional mock! |
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.
Code looks good. Documentation is clever. Visuals looks clean.
I might include a more basic example showing them in their default states. Might also be good to lay them out in a horizontal version just so people have a copy/paste example. Then your current doc example exists as the... ok, now here's all the stuff you can do to configure them.
Not necessary for this PR, just a thought so we follow our existing doc patterns.
Ok Can I get a quick re-check? Sorry, and thanks in advance! |
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.
Loaded again locally. Looks good. Thanks for switching up the docs.
This adds the
EuiButtonFacet
component seen in this mockup.The documentation also explains how to lay them out.
cc @bevacqua