Tag size #5914
Tag size #5914
Conversation
src/js/themes/base.js
Outdated
| @@ -1489,6 +1489,23 @@ export const generate = (baseSpacing = 24, scale = 6) => { | |||
| margin: { right: 'xsmall' }, | |||
| }, | |||
| separator: ' : ', | |||
| size: { | |||
| xsmall: { | |||
| pad: { horizontal: 'small' }, | |||
There was a problem hiding this comment.
Does this padding align with Button for its sizes?
There was a problem hiding this comment.
Tag has less horizontal padding (that's how it was spec'd in the figma). Button also calculates it as something like ${baseSpacing / 4 - borderWidth} etc rather than using tshirt sizes which we could do if needed, especially once the Figma details are decided.
There was a problem hiding this comment.
I tried to align them to be more consistent. They are a little different than button but that seems reasonable since they better fit the original Tag "medium" design.
| <Text size={size} weight="bold">{size}</Text> | ||
| <Tag name="Name" value="Value" size={size}/> | ||
| <Tag value="Value" size={size}/> | ||
| <Tag value="Value" size={size} onRemove={()=>{}}/> |
There was a problem hiding this comment.
onRemove for size "xsmall" seems bad for accessibility. The clickable target is only 12x12px. Wondering if we shouldn't support onRemove for this size OR at least advise away from it for Design System related patterns.
There was a problem hiding this comment.
Agree. I don't think xsmall makes sense to have in a case where you want onRemove. I bigger target may make xsmall not really any smaller than small if it does have onRemove. Either way these values were a guess just to get some discussion started :)
There was a problem hiding this comment.
Changed the remove button for xsmall to be like the small one. It seems to fit fine and looks decent.
There was a problem hiding this comment.
the xsmall remove looks better
| "size": "small", | ||
| }, | ||
| "pad": Object { | ||
| "horizontal": "small", |
There was a problem hiding this comment.
This horizontal pad looks disproportionally large than the horizontal pad on other sizes -- is that intentional?
There was a problem hiding this comment.
It does look a little big but xsmall is too small so it probably needs to be a pixel value (which I think we'd do anyway to be more like how button's padding is done.)
There was a problem hiding this comment.
I re-did the padding on all but medium so they align better with medium (which was the one originally in Figma.)
| @@ -11,23 +11,27 @@ import { Text } from '../Text'; | |||
|
|
|||
| import { StyledRemoveButton, StyledTagButton } from './StyledTag'; | |||
|
|
|||
| const Tag = forwardRef(({ name, value, onRemove, onClick, ...rest }, ref) => { | |||
| const Tag = forwardRef( | |||
| ({ name, value, size, onRemove, onClick, ...rest }, ref) => { | |||
There was a problem hiding this comment.
can we add size to the grommet docs
What does this PR do?
Add a
sizeproperty to TagWhere should the reviewer start?
Tag.js
What testing has been done on this PR?
Storybook example and jest testcase
How should this be manually tested?
Storybook Type/Tag/Size example
Do Jest tests follow these best practices?
screenis used for querying.userEventis used in place offireEvent.asFragment()is used for snapshot testing.Any background context you want to provide?
Greenlake Marketplace designs want a smaller tag so they don't overwhelm and distract from the content on their product cards.
What are the relevant issues?
Screenshots (if appropriate)
Do the grommet docs need to be updated?
Yes
Should this PR be mentioned in the release notes?
Yes
Is this change backwards compatible or is it a breaking change?
Backwards compatible