-
Notifications
You must be signed in to change notification settings - Fork 26
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
FCT 1079 - filters Chip
#2937
FCT 1079 - filters Chip
#2937
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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.
Looks like it does the thing!
@ByronDWall thanks! I just realized we introduced |
and one more tiny thing. In the designs the overall height is 24px, in the PR it's 22 |
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.
Left a really tiny nit. Otherwise 👌🏽
return ( | ||
<> | ||
<div>{props.label}</div> | ||
<ul style={{ display: 'flex', gap: '8px' }}> |
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.
Do you think we should use spacing20 here?
<ul style={{ display: 'flex', gap: '8px' }}> | |
<ul style={{ display: 'flex', gap: designTokens.spacing20 }}> |
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.
sure, but jsyk, this code is meant to be temporary
color: ${designTokens.colorPrimary20}; | ||
background-color: ${designTokens.colorPrimary95}; | ||
padding: 0 ${designTokens.spacing20}; | ||
border-radius: calc( |
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.
smooth!
@FilPob how do you feel about using
|
dea9379
to
3f5ed31
Compare
@ByronDWall yeah lets do that! thanks |
@FilPob height and colors should now be as specified, thx! |
…reviewers can see component in preview storybook
9c6c9cf
to
aa7adc2
Compare
This PR implements the
Chip
component with necessary styles and state as specified in figma.The
Chip
is implemented as anli
, and should be used as a child of aul
component, as it is meant to list the selected filter values in aFilterMenu
.Component Enhancements:
label
andisDisabled
props, and styled using@commercetools-uikit/design-system
and@emotion/react
.Chip
component to check for thelabel
prop and role attribute.PLEASE NOTE: The Filters storybook must be updated to have a local-dev tag before merging this PR to main so that the Filters docs do not get published to the prod site, these docs are published for this PR as a courtesy to the reviewers.
Chip can be seen here