acl-services / paprika

🌶 A robust + accessible UI component library for React applications by Galvanize.
MIT License
54 stars 9 forks source link

UXD-1779 Button align with UI kit #1161

Closed ShanAtDiligent closed 2 years ago

ShanAtDiligent commented 2 years ago

Purpose 🚀

update Button style to align with UI Kit

https://aclgrc.atlassian.net/browse/UXD-1779

Notes ✏️

Storybook 📕

http://storybooks.highbond-s3.com/paprika/UXD-1779-Button-Align-With-UI-Kit

Screenshots 📸

Screen Shot 2021-10-27 at 1 46 23 PM

Screen Shot 2021-10-27 at 11 00 38 AM Screen Shot 2021-10-27 at 11 00 32 AM Screen Shot 2021-10-27 at 10 44 35 AM

changeset-bot[bot] commented 2 years ago

🦋 Changeset detected

Latest commit: 0505e267c1264c0515a15ac1a6c3545bf3d51e91

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 35 packages | Name | Type | | --------------------------- | ----- | | @paprika/button | Patch | | @paprika/button-group | Patch | | @paprika/panel | Patch | | @paprika/breadcrumbs | Patch | | @paprika/action-bar | Patch | | @paprika/calendar | Patch | | @paprika/collapsible-card | Patch | | @paprika/collapsible-text | Patch | | @paprika/confirmation | Patch | | @paprika/copy-input | Patch | | @paprika/data-field | Patch | | @paprika/data-grid | Patch | | @paprika/data-header | Patch | | @paprika/dialog-actions | Patch | | @paprika/filter | Patch | | @paprika/input | Patch | | @paprika/list-box | Patch | | @paprika/modal | Patch | | @paprika/overflow-menu | Patch | | @paprika/side-navigation | Patch | | @paprika/sortable | Patch | | @paprika/status-tracker | Patch | | @paprika/table | Patch | | @paprika/tag | Patch | | @paprika/takeover | Patch | | @paprika/toast | Patch | | @paprika/uploader | Patch | | @paprika/date-picker | Patch | | @paprika/date-range-picker | Patch | | @paprika/inline-editors | Patch | | @paprika/date-input | Patch | | @paprika/search | Patch | | @paprika/time-picker | Patch | | @paprika/list-box-browser | Patch | | @paprika/list-box-with-tags | Patch |

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

mikrotron commented 2 years ago

Looking at Screener again, and although we don't have any coverage for the <Tag> component, I noticed with the <ListBoxWithTags> that the "remove" button for the <Tags> now looks very close to the right edge.

Screen Shot 2021-10-29 at 2 13 05 PM

Actually the button always was this close to the edge, but when the icon was smaller it had more visible padding. Not sure if @jonathanjakimon thinks this should be adjusted?

mikrotron commented 2 years ago

Found another unintended change thanks to Screener. In the <Breadcrumbs> component, the "back" arrow has grown: https://screener.io/v2/states/H1OkdIZtV.acl-services-paprika/UXD-1779-Button-Align-With-UI-Kit/1280x1024/Chrome/navigationbreadcrumbsbackyardtests-screener-0

I think I remember reading that the size of this little left pointing caret is actually an exception to the 20px size rule, so we should retain the existing size in this case (14px).

jonathanjakimon commented 2 years ago

@mikrotron About the tag we will use another icon and the padding will be 4px on the right + natural padding of the icon in its 20px box.

image

mikrotron commented 2 years ago

@mikrotron About the tag we will use another icon and the padding will be 4px on the right + natural padding of the icon in its 20px box.

image

Oh! Okay, if it's going to change this much then maybe we should just leave it as is for now.

ShanAtDiligent commented 2 years ago

Found another unintended change thanks to Screener. In the <Breadcrumbs> component, the "back" arrow has grown: https://screener.io/v2/states/H1OkdIZtV.acl-services-paprika/UXD-1779-Button-Align-With-UI-Kit/1280x1024/Chrome/navigationbreadcrumbsbackyardtests-screener-0

I think I remember reading that the size of this little left pointing caret is actually an exception to the 20px size rule, so we should retain the existing size in this case (14px).

Hi @mikrotron I done another test locally after run npx lerna bootstrap and look slike the arrows on <Breadcrumbs> aren't affected by these changes.

mikrotron commented 2 years ago

While reviewing the Screener changes I notice that this use case of applying a custom color via CSS to the <Button.Icon> will no longer work:

https://github.com/acl-services/paprika/blob/master/packages/Button/stories/variations/IconButtonVariations.js#L75-L81

I think this is fine. It makes more sense to apply a custom color directly to the <Icon> anyway.

But... it has let to a small regression in the <Breadcrumbs> since this is the technique being applied:

https://github.com/acl-services/paprika/blob/master/packages/Breadcrumbs/src/components/ExpandButton/ExpandButton.styles.js#L7