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

Update overlay token values #646

Merged
merged 3 commits into from
Oct 20, 2022
Merged

Conversation

jorytindall
Copy link
Contributor

📌 Summary

If merged this PR will update the values of the token-surface-overlay-box-shadow and token-elevation-overlay-box-shadow to match the visual language updates made during the dialog work.

As a note: the values being revised were defined prior to having a context to use them in, upon establishing the context of the dialog and modal components some necessary updates were made to soften and maintain consistency with the rest of the elevation and surface box shadow styles.

👀 How to review

👉 Review commit-by-commit
👉 Review by files changed

Reviewer's checklist:

  • +1 Percy if applicable
  • Confirm that PR has a changelog update via Changesets if needed

💬 Please consider using conventional comments when reviewing this PR.

@vercel
Copy link

vercel bot commented Oct 20, 2022

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

Name Status Preview Comments Updated
hds-components ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Oct 20, 2022 at 4:56PM (UTC)
hds-flight-website ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Oct 20, 2022 at 4:56PM (UTC)
hds-website ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Oct 20, 2022 at 4:56PM (UTC)

@changeset-bot
Copy link

changeset-bot bot commented Oct 20, 2022

🦋 Changeset detected

Latest commit: ecbe26d

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

This PR includes changesets to release 2 packages
Name Type
@hashicorp/design-system-tokens Minor
@hashicorp/design-system-components 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

@jorytindall
Copy link
Contributor Author

jorytindall commented Oct 20, 2022

@didoo marking this as a draft since I haven't made updates to the token library before. Would love your feedback, for context the changes that were made can be found in the Figma Foundations here.

Since this is a visual update and change in the values, does this need a changeset?

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.

@jorytindall all good for me! great job! 🙌

yes, you need to add a changeset, instructions here: https://github.com/hashicorp/design-system/blob/main/packages/components/NEW-COMPONENT-CHECKLIST.md#component-review

add a changelog update via Changesets if needed using the command yarn changeset (in the project root)

@@ -1,6 +1,6 @@
/**
* Do not edit directly
* Generated on Sat, 16 Jul 2022 14:15:33 GMT
* Generated on Wed, 19 Oct 2022 16:34:46 GMT
Copy link
Contributor

Choose a reason for hiding this comment

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

[nitpick] Not an issue per se, but for general knowledge: when you run a yarn build for the tokens, the dates are updated in every generated file. As a matter or reducing the overall noise, I tend to not commit these files (with only the date changed) but it's OK anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

not really urgent to address, but how hard would it be to not update these dates if the file hasn't actually changed?

Copy link
Contributor

Choose a reason for hiding this comment

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

Last time I looked at it was not so obvious (also see: amzn/style-dictionary#768)

@didoo
Copy link
Contributor

didoo commented Oct 20, 2022

FYI I have already approved the failing visual regression test (it's expected to be like this)
screenshot_1960

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.

None yet

3 participants