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

Cleanup color tokens #2419

Merged
merged 7 commits into from
Sep 16, 2024
Merged

Cleanup color tokens #2419

merged 7 commits into from
Sep 16, 2024

Conversation

jorytindall
Copy link
Contributor

@jorytindall jorytindall commented Sep 13, 2024

📌 Summary

As discovered in the Figma v2.0 Library project:

  • Updates the icon colors in the Copy::Button to match Figma and the interactive states of the component
  • Updates the Stepper::Indicator::Step to use a surface token instead of a palette token.
  • Fixes a bug in the secondary Copy::Snippet where the icon had it's own interactive state, now preventing the color of the icon from changing with the component status = success or error. This matches the experience for the primary variant.

📸 Screenshots

No visual change in the Stepper, but there is a visual change in the CopyButton:

Before
Screenshot 2024-09-13 at 12 21 08 PM

After
Screenshot 2024-09-13 at 12 20 23 PM

🔗 External links

Jira ticket: HDS-3845, HDS-3845
Figma file: CopyButton


👀 Component checklist

💬 Please consider using conventional comments when reviewing this PR.

@jorytindall jorytindall requested review from LilithJames-HDS and a team September 13, 2024 19:23
Copy link

vercel bot commented Sep 13, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
hds-showcase ✅ Ready (Inspect) Visit Preview Sep 16, 2024 5:33pm
hds-website ✅ Ready (Inspect) Visit Preview Sep 16, 2024 5:33pm

…teractive state when status is success or error
@didoo didoo self-requested a review September 13, 2024 20:00
&:hover,
&.mock-hover {
background-color: var(--token-color-surface-interactive);
border-color: var(--token-color-border-strong);

// Change the color of the icon in hover state, except if the component has a status of success or error
.hds-copy-snippet__icon:not(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure if this is the cleanest solution, but it does prevent some repetition in declaring the hover states.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Throws an error in the linter, so I'm thinking this is not the best approach.

Copy link
Contributor

Choose a reason for hiding this comment

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

not necessarily. I think you have a linter different from what is used in the repo. I'll have a look on monday

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I reworked this a bit passed a comma-separated list of classes rather than chaining multiple :not() pseudo classes together which seems to satisfy the linter.

Copy link
Contributor

Choose a reason for hiding this comment

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

[suggestion] instead of doing all these not() overrides, I would follow the same approach used for the Copy::Button which is to increase the specificity for the "success/error" state classes (and in this case, since it would be used twice, I would extract it to a local mixin):

@mixin hds-copy-snippet-success-error() {
  &.hds-copy-snippet--status-success {
    background-color: var(--token-color-surface-interactive);

    .hds-copy-snippet__icon {
      color: var(--token-color-foreground-success);
    }
  }

  &.hds-copy-snippet--status-error {
    background-color: var(--token-color-surface-interactive);

    .hds-copy-snippet__icon {
      color: var(--token-color-foreground-critical);
    }
  }
}

.hds-copy-snippet {
  @include hds-focus-ring-with-pseudo-element();

  display: flex;
  gap: 8px;
  align-items: center;
  justify-content: space-between;
  padding: 6px 4px;
  text-align: left;
  border: 1px solid transparent;
  border-radius: 5px;
  cursor: pointer;
}

.hds-copy-snippet--color-primary {
  color: var(--token-color-foreground-action);
  background-color: transparent;

  &:hover,
  &.mock-hover {
    color: var(--token-color-foreground-action-hover);
    background-color: var(--token-color-surface-interactive);
    border-color: var(--token-color-border-strong);
  }

  &:active,
  &.mock-active {
    color: var(--token-color-foreground-action-active);
    background-color: var(--token-color-surface-interactive-active);
    border-color: var(--token-color-border-strong);
  }

  @include hds-copy-snippet-success-error();
}

.hds-copy-snippet--color-secondary {
  color: var(--token-color-foreground-primary);
  background-color: transparent;

  .hds-copy-snippet__icon {
    color: var(--token-color-foreground-action);
  }

  &:hover,
  &.mock-hover {
    background-color: var(--token-color-surface-interactive);
    border-color: var(--token-color-border-strong);

    .hds-copy-snippet__icon {
      color: var(--token-color-foreground-action-hover);
    }
  }

  &:active,
  &.mock-active {
    background-color: var(--token-color-surface-interactive-active);
    border-color: var(--token-color-border-strong);

    .hds-copy-snippet__icon {
      color: var(--token-color-foreground-action-active);
    }
  }

  @include hds-copy-snippet-success-error();
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a great suggestion, much cleaner!

Copy link
Contributor

@didoo didoo left a comment

Choose a reason for hiding this comment

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

Left a suggestion. Technically not a blocker so I am approving the PR 👍

@jorytindall jorytindall merged commit 2bd55b7 into main Sep 16, 2024
14 checks passed
@jorytindall jorytindall deleted the jt-color-token-cleanup branch September 16, 2024 17:43
@hashibot-hds hashibot-hds mentioned this pull request Sep 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants