feat(MultipleFileUpload): apply tokens#6214
Conversation
|
Preview: https://patternfly-pr-6214.surge.sh A11y report: https://patternfly-pr-6214-a11y.surge.sh |
There was a problem hiding this comment.
Great work. Couple little things.
- On the progress bars there is an icon button with
fa-times-altinstead offa-times - In the examples the file name in the progress bar looks like a link? Should we update the Figma component to be a link. Wasn't sure if that title sends them to the file when it is fully uploaded.
|
@andrew-ronaldson I don't think it should be a link, I just missed updating the text color in the progress. Updated the icon as well. |
There was a problem hiding this comment.
Nailed it. Thanks
There was a problem hiding this comment.
Will take another look pending any updates following discussion that was had about icon/font sizes etc
There was a problem hiding this comment.
Looks like we need to make sure the title text is using the heading font-family, size, and weight tokens, and they match what's in the design.
There was a problem hiding this comment.
Just a few comments, the title size tokens would be good to verify prior to merging.
Also a note for design, it should be easy enough to adjust this close button position if we wanted it to align with the content. Here's what it looks like now:
If we just push the button up by the value of its top padding:
| // title text | ||
| --#{$multiple-file-upload}__title-text--Color: var(--pf-t--global--text--color--regular); | ||
| --#{$multiple-file-upload}__title-text--FontFamily: var(--pf-t--global--font--family--heading); | ||
| --#{$multiple-file-upload}__title-text--FontSize: var(--pf-t--global--font--size--lg); |
There was a problem hiding this comment.
I think this should use the small heading token?
| --#{$multiple-file-upload}__title-text--FontSize: var(--pf-t--global--font--size--lg); | |
| --#{$multiple-file-upload}__title-text--FontSize: var(--pf-t--global--font--size--heading--sm); |
| --#{$multiple-file-upload}--m-horizontal__title--Gap: var(--#{$pf-global}--spacer--xs); | ||
| --#{$multiple-file-upload}--m-horizontal__title--Gap: var(--pf-t--global--spacer--sm); | ||
| --#{$multiple-file-upload}--m-horizontal__title-icon--FontSize: var(--pf-t--global--font--size--md); | ||
| --#{$multiple-file-upload}--m-horizontal__title-text--FontSize: var(--pf-t--global--font--size--md); |
There was a problem hiding this comment.
looks like this should use the xs heading token?
| --#{$multiple-file-upload}--m-horizontal__title-text--FontSize: var(--pf-t--global--font--size--md); | |
| --#{$multiple-file-upload}--m-horizontal__title-text--FontSize: var(--pf-t--global--font--size--heading--xs); |
| --#{$multiple-file-upload}__title--Gap: var(--#{$multiple-file-upload}--m-horizontal__title--Gap); | ||
| --#{$multiple-file-upload}__title-icon--FontSize: var(--#{$multiple-file-upload}--m-horizontal__title-icon--FontSize); | ||
| --#{$multiple-file-upload}__title-text--FontSize: var(--#{$multiple-file-upload}--m-horizontal__title-text--FontSize); | ||
| --#{$multiple-file-upload}__title-text-separator--FontFamily: var(--#{$multiple-file-upload}--m-horizontal__title-text-separator--FontFamily); |
There was a problem hiding this comment.
This can be a follow up, but I wonder if this text belongs outside of the title text element. That would allow us to not have to reset these font styles for just that element.
|
🎉 This PR is included in version 6.0.0-alpha.65 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Closes #6210
Figma designs for file upload
Convenience link: https://patternfly-pr-6214.surge.sh/components/file-upload/multiple-file-upload