Adding Tag Component #225
Adding Tag Component #225
Conversation
| <Button | ||
| className="rc-tag-remove-button" | ||
| onClick={() => onClick()} | ||
| icon="close" | ||
| /> |
There was a problem hiding this comment.
If we wanted to be even more accessible here, we could add an aria-label to the button (which makes me realize we should do that in the other icon button examples).
| <Button | |
| className="rc-tag-remove-button" | |
| onClick={() => onClick()} | |
| icon="close" | |
| /> | |
| <Button | |
| className="rc-tag-remove-button" | |
| onClick={() => onClick()} | |
| icon="close" | |
| aria-label="Remove tag" | |
| /> |
There's probably some way to use aria-labelledby to create a relationship between the Tag text and the button, but that might require id generation. I think the above use of aria-label would suffice.
|
|
||
| const propTypes = { | ||
| /** Tag text or other content */ | ||
| children: node.isRequired, |
There was a problem hiding this comment.
Unless we know we need to accept arbitrary content, can we add something like a label string prop instead? (I could see the future use of an icon on the left, but we can either add a new prop for that when the time comes or readdress children.) To be clear, I'm usually in favor of adding the "escape hatch" of adding children as a prop to increase flexibility, but in this case, we're going to need to style the text and a separate prop would leave the door open for adding aria-labelledby.
| .rc-tag-text { | ||
| color: $puppet-white; | ||
| height: fit-content; | ||
| padding-left: 8px; | ||
| padding-right: 4px; | ||
| } |
There was a problem hiding this comment.
It looks like the spec in Figma uses a font of Calibre, 14px, bold (like h5) instead of the default Open Sans font. This obviously isn't a header though, so I'd like to confirm the typography with @Sigler. There's a precedent to using Calibre in non-headers with form labels, but if we're going to continue to do things like that, we need a simple way of defining and using those typographic styles.
There was a problem hiding this comment.
It's the same font as font as button, so let's just add @include puppet-type-button() like in _button.scss.
Lukeaber
force-pushed
the
PDS-452-Create-Tag-Component
branch
3 times, most recently
from
e9cebd1 to
4e97e55
Compare
Lukeaber
force-pushed
the
PDS-452-Create-Tag-Component
branch
from
4e97e55 to
9d381af
Compare
| .rc-tag-text { | ||
| color: $puppet-white; | ||
| height: fit-content; | ||
| padding-left: 8px; |
There was a problem hiding this comment.
The left padding also seems too tight to me (though I can see an 8px in Figma). I'd suggest copying the padding of button:
| padding-left: 8px; | |
| padding-left: 15px; |
|
|
https://www.figma.com/file/9RzmjE0XYWWADWLKOJP5mR/PDS-UI-Components?node-id=135%3A1021
Ive added the tag component, styles, tests and a first run at docs