Skip to content

refactor(swatch): use core tokens #2570

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

Merged
merged 2 commits into from
Oct 20, 2022
Merged

refactor(swatch): use core tokens #2570

merged 2 commits into from
Oct 20, 2022

Conversation

yosevu
Copy link
Contributor

@yosevu yosevu commented Sep 8, 2022

Description

Expected Changes

  • Swatch group density swatch gap differences e.g.

Current

Screen Shot 2022-10-04 at 4 58 29 AM

Core tokens

Screen Shot 2022-10-04 at 4 58 48 AM

Unexpected Changes

There are unexpected changes in SWC due to HTML and CSS changes to the Swatch component in CSS including:

  • is-image class for transparent backgrounds on Image, Gradient, and Mixed Value swatches
  • selected swatch states do not work on all variants in SWC (I have not investigated this yet)
  • Swatch border relies on hardcoded tokens.

Here is one example:

I hard coded this token since I couldn't find one to use: https://github.com/adobe/spectrum-css/pull/1501/files#diff-cc50ec1bfa218207591ac782352b4f5894b71d81450b2c74fa9004ca1dfca665R22

Screen Shot 2022-10-04 at 5 06 53 AM

Screen Shot 2022-10-04 at 5 06 58 AM

Related issue(s)

Motivation and context

How has this been tested?

  • Test case 1
    1. Go here
    2. Do this
  • Test case 2
    1. Go here
    2. Do this

Screenshots (if appropriate)

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Chore (minor updates related to the tooling or maintenance of the repository, does not impact compiled assets)

Checklist

  • I have signed the Adobe Open Source CLA.
  • My code follows the code style of this project.
  • If my change required a change to the documentation, I have updated the documentation in this pull request.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • I have reviewed at the Accessibility Practices for this feature, see: Aria Practices

Best practices

This repository uses conventional commit syntax for each commit message; note that the GitHub UI does not use this by default so be cautious when accepting suggested changes. Avoid the "Update branch" button on the pull request and opt instead for rebasing your branch against main.

@yosevu
Copy link
Contributor Author

yosevu commented Sep 12, 2022

Can I include swatchgroup 2.0.0-beta.0 adobe/spectrum-css#1505 in this PR @pfulton @Westbrook? I don't see a separate package for it.

@Westbrook
Copy link
Contributor

Yep, that'll be great. We merge them back together in our projects, so this is the right place!

@yosevu yosevu force-pushed the yosevu/swatch-core-tokens branch from 1721ed6 to 0fe867e Compare September 13, 2022 20:45
@github-actions
Copy link

github-actions bot commented Sep 13, 2022

Tachometer results

Chrome

swatch permalink

Version Bytes Avg Time vs remote vs branch
npm latest 280 kB 31.77ms - 31.93ms - faster ✔
0% - 2%
0.14ms - 0.50ms
branch 282 kB 32.00ms - 32.33ms slower ❌
0% - 2%
0.14ms - 0.50ms
-
Firefox

swatch permalink

Version Bytes Avg Time vs remote vs branch
npm latest 280 kB 111.78ms - 122.70ms - unsure 🔍
-3% - +10%
-3.59ms - +11.35ms
branch 282 kB 108.26ms - 118.46ms unsure 🔍
-10% - +3%
-11.35ms - +3.59ms
-

@yosevu yosevu force-pushed the yosevu/swatch-core-tokens branch from aaaa3ed to 2cee395 Compare September 28, 2022 18:42
@Westbrook Westbrook marked this pull request as ready for review October 19, 2022 02:13
@Westbrook Westbrook force-pushed the yosevu/swatch-core-tokens branch from 2cee395 to 67c26c7 Compare October 19, 2022 21:44
@Westbrook Westbrook force-pushed the yosevu/swatch-core-tokens branch from 85c5b93 to 8b6f98e Compare October 20, 2022 16:41
@Westbrook Westbrook merged commit c5a5aea into main Oct 20, 2022
@Westbrook Westbrook deleted the yosevu/swatch-core-tokens branch October 20, 2022 18:20
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.

2 participants