Skip to content
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

style(Tag): Update Tag colors #2911

Merged
merged 25 commits into from
Aug 5, 2024
Merged

style(Tag): Update Tag colors #2911

merged 25 commits into from
Aug 5, 2024

Conversation

LinKCoding
Copy link
Contributor

@LinKCoding LinKCoding commented Jul 20, 2024

Overview

Updates tags to use the correct color tokens.
Current implementation of tags somewhat uses ColorMode and needs to invert the colors to work.

Major Changes:

  1. removed DismissButton from TagWrapper
  2. TagWrapper => TagLabelWrapper
  3. ^ necessary to remove the background behind DimissButton
  4. Tag is no longer a Background, is now a Box
  5. Changed the BaseButton to an IconButton (to gain access to ToolTip properties)
  6. The gray variant should have updated colors (and for hover state too)

Updates tags to use the correct color tokens.

PR Checklist

Testing Instructions

  1. Go to the Storybook preview and the tags page ?path=/docs/atoms-tag--tag-variants
  2. Look over the tags implementation, compare against the figma

    Mono
  3. Go to the tengu preview env
  4. Go to Pricing > Individuals > select "Try Pro for free"
  5. Create an account
  6. At checkout, click on the "Have a promo code?" link
  7. Add in test50
  8. Check that the tag renders correctly.

    Monolith
  9. go to the preview (without portal app), log in as an admin
  10. Go to the admin page and create a test user that has a Pro subscription + billed monthly (you can also use this link:
    https://pr-38096-monolith.dev-eks.codecademy.com/admin/test_user?prefill_id=66ab9a074e03aa000159fe35)
    ^ the settings is important because we need to upgrade the user (and can't seem to upgrade free or pro-lite users)
  11. Impersonate the test user
  12. go to "Account + Billing" > "Payment And Plan" tab
  13. Select "Change Plan" and opt to upgrade to bill yearly
  14. in the address bar append the query string: ?coupon=test50
  15. see that the tag is rendered
  16. ...
  17. Profit

Screenshots:
Mono:
image

Monolith:
image

PR Links and Envs

Repository PR Link PR Env
Monolith Monolith PR Monolith Env
Portal Portal Link Portal Env
Another Repo Another Link Another Env

@dreamwasp
Copy link
Contributor

dreamwasp commented Jul 23, 2024

image

this looks great, i don't see the line on zoom at all so i think it's just a weird pixel thing 🤷 only issue i'm seeing is this strange focus state bug (prob some inherited style 🤔 )

@LinKCoding
Copy link
Contributor Author

this looks great, i don't see the line on zoom at all so i think it's just a weird pixel thing 🤷 only issue i'm seeing is this strange focus state bug (prob some inherited style 🤔 )

Nice, thanks! - I realized that focus states are a little messed up after I implemented my changes, gonna take some time to fix that
(Just need to do the truncated tag for some reason the outline doesn't reach the full component)

@LinKCoding LinKCoding marked this pull request as ready for review July 26, 2024 15:41
@LinKCoding LinKCoding requested a review from a team as a code owner July 26, 2024 15:41
Copy link
Contributor

@dreamwasp dreamwasp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these look great! only thing i'm noticing is something slightly off with the focus styles on hover of one of the examples - the focus styles seem to be taking up the full width of their container?

image

if this is only happening in this example and not in actual code i'm happy to ✅ so i'm gonna take a look at mono now~

Comment on lines +28 to +29
width: '100%',
maxWidth: 'fit-content',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

chef's kiss!~

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lol this was your code! 😂

@LinKCoding
Copy link
Contributor Author

these look great! only thing i'm noticing is something slightly off with the focus styles on hover of one of the examples - the focus styles seem to be taking up the full width of their container?

image

if this is only happening in this example and not in actual code i'm happy to ✅ so i'm gonna take a look at mono now~

I see the issue — I'm guessing to add more spacing around tags, there was a .5 rem margin added around the tag itself which makes the focus outline bigger.

I've added a box around the tag now to include this padding and this should fix the issue.

Thanks for catching!!

Copy link
Contributor

@dreamwasp dreamwasp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is super weird but it looks like the remove buttons on multiselect tags lost their right border radius somehow 🤔
image

Comment on lines +59 to +61
[hoverAndFocus]: {
color: 'background',
bg: 'secondary-hover',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔥

packages/gamut/src/Tag/styles.tsx Outdated Show resolved Hide resolved
@dreamwasp
Copy link
Contributor

The SelectDropdown multiselect tag border radius are still a little off (2px v 4px in tags)- i think borderRadius got moved out of tagBaseStyles or something in the multiValue object so it's not making its way there anymore. you should be able to fix it by adding borderRadius 4px [here] (

)
image

Copy link
Contributor

@dreamwasp dreamwasp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ignore this, my local storage was out of wack, looks great!

not sure when but at some point in the last couple commits the default button styling came back on focus 🤔

image

@@ -174,6 +174,7 @@ export const getMemoizedStyles = (
...tagBaseStyles,
cursor: 'pointer',
background: theme.colors.background,
borderRadius: '4px',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you could grab this from the tagBorderRadius var just so we're ensuring it stays consistent!

Copy link
Contributor

@dreamwasp dreamwasp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these look wonderful! beautiful job ✨

@codecademydev
Copy link
Collaborator

📬Published Alpha Packages:

@codecademy/[email protected]
@codecademy/[email protected]
@codecademy/[email protected]

@codecademydev
Copy link
Collaborator

🚀 Styleguide deploy preview ready!

https://66b0f5496e8dc3241d410074--gamut-preview.netlify.app

Deploy Logs

@LinKCoding LinKCoding added the Ship It :shipit: Automerge this PR when possible label Aug 5, 2024
@codecademydev codecademydev merged commit c6bc852 into main Aug 5, 2024
17 of 18 checks passed
@codecademydev codecademydev removed the Ship It :shipit: Automerge this PR when possible label Aug 5, 2024
@codecademydev codecademydev deleted the kl-gm-778 branch August 5, 2024 15:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants