spectrum-web-components icon indicating copy to clipboard operation
spectrum-web-components copied to clipboard

feat(reactive-controllers): Migrate to Colorjs from Tinycolor

Open blunteshwar opened this issue 1 year ago โ€ข 4 comments

This PR marks the initial steps in our color library migration for Spectrum Web Components. As part of this effort, I have implemented an abstraction layer that includes the following key elements:

Color Types: Defined standardized types to ensure consistent handling of color data across components. Transformation Functions: Created functions to handle color conversions and transformations, making it easier to work with various color formats. This abstraction layer is designed to facilitate a smoother migration process and minimize the impact on our consumers. By isolating the color logic, we can easily adapt to future changes in the underlying color library.

Stay tuned for more detailsโ€”there's much more to come! ๐Ÿš€

Related issue(s)

#3950 #3883 #3655 #3071 #3058

Motivation and context

  • [ ] Did it pass in Desktop?
  • [ ] Did it pass in Mobile?
  • [ ] Did it pass in iPad?

Types of changes

  • [ ] Bug fix (non-breaking change which fixes an issue)
  • [x] 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

  • [x] I have signed the Adobe Open Source CLA.
  • [x] My code follows the code style of this project.
  • [x] If my change required a change to the documentation, I have updated the documentation in this pull request.
  • [x] I have read the CONTRIBUTING document.
  • [x] 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.

blunteshwar avatar Sep 02 '24 08:09 blunteshwar

Branch preview

Review the following VRT differences

When a visual regression test fails (or has previously failed while working on this branch), its results can be found in the following URLs:

If the changes are expected, update the current_golden_images_cache hash in the circleci config to accept the new images. Instructions are included in that file. If the changes are unexpected, you can investigate the cause of the differences and update the code accordingly.

github-actions[bot] avatar Sep 04 '24 14:09 github-actions[bot]

Pull Request Test Coverage Report for Build 13498253698

Details

  • 818 of 824 (99.27%) changed or added relevant lines in 10 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage decreased (-0.001%) to 98.178%

Changes Missing Coverage Covered Lines Changed/Added Lines %
packages/color-area/src/ColorArea.ts 74 75 98.67%
tools/reactive-controllers/src/ColorController.ts 693 698 99.28%
<!-- Total: 818 824
Files with Coverage Reduction New Missed Lines %
packages/color-area/src/ColorArea.ts 1 95.65%
<!-- Total: 1
Totals Coverage Status
Change from base Build 13496959333: -0.001%
Covered Lines: 33328
Relevant Lines: 33765

๐Ÿ’› - Coveralls

coveralls avatar Sep 04 '24 14:09 coveralls

Tachometer results

Currently, no packages are changed by this PR...

github-actions[bot] avatar Sep 04 '24 15:09 github-actions[bot]

Lighthouse scores

Category Latest (report) Main (report) Branch (report)
Performance 0.99 0.98 0.98
Accessibility 1 1 1
Best Practices 1 1 1
SEO 1 0.92 0.92
PWA 1 1 1
What is this?

Lighthouse scores comparing the documentation site built from the PR ("Branch") to that of the production documentation site ("Latest") and the build currently on main ("Main"). Higher scores are better, but note that the SEO scores on Netlify URLs are artifically constrained to 0.92.

Transfer Size

Category Latest Main Branch
Total 243.718 kB 231.29 kB ๐Ÿ† 231.589 kB
Scripts 60.028 kB 54.245 kB ๐Ÿ† 54.464 kB
Stylesheet 47.476 kB 42.63 kB ๐Ÿ† 42.647 kB
Document 6.237 kB 5.466 kB ๐Ÿ† 5.493 kB
Font 126.988 kB 126.597 kB ๐Ÿ† 126.633 kB

Request Count

Category Latest Main Branch
Total 52 52 52
Scripts 41 41 41
Stylesheet 5 5 5
Document 1 1 1
Font 2 2 2

github-actions[bot] avatar Sep 17 '24 10:09 github-actions[bot]

@blunteshwar Can we add how have PR reviewers manually test this in the instructions? Something along the lines of going to storybook and use the color components with different values?

nikkimk avatar Nov 20 '24 14:11 nikkimk

@blunteshwar Can we add how have PR reviewers manually test this in the instructions? Something along the lines of going to storybook and use the color components with different values?

Since this is not a bug fix but a migration, I don't think there is much to test. Although this migration solves a lot of bugs, many which have been reported and many which haven't been reported yet it is difficult to add a link to test them all. But if you want to test any color-component in a general manner I can attach the storybook link in the pr description.

blunteshwar avatar Nov 21 '24 06:11 blunteshwar

Just a minor change or two in the docs and we're good to go.

Can you point out those changes in docs?

blunteshwar avatar Nov 25 '24 13:11 blunteshwar

โš ๏ธ No Changeset found

Latest commit: ef73ff4574b7a566630f6cea30e954f63f8beeaa

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

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

changeset-bot[bot] avatar Nov 26 '24 12:11 changeset-bot[bot]

Before we merge this to main, Can we make sure we add the context of how this should be added to the existing downstream products. Say if an product is already using a TinyColor library and we want to ship this version how will the migration look like? Did we create any documentation as such to cater these cases? We should bring this change in a Office Hours and discuss on its usage before we make this a final go in our main

Rajdeepc avatar Nov 27 '24 08:11 Rajdeepc

I noticed that the "Joint" ColorArea story has an issue where moving the hue ColorSlider does not change the background gradient of the ColorArea. However, the ColorHandle does stay in sync with the correct hue, and the components color value does have the correct hue as well.

I thought this issue would be addressed by this transition to Colorjs, but after pulling down this PR I noticed that the issue actually persists. I find this issue a bit strange because the video attachment to https://github.com/adobe/spectrum-web-components/issues/3071 includes a correctly functioning ColorArea.

Let me know if I should open an Issue for this by itself.

image

blaabaer avatar Jan 08 '25 00:01 blaabaer

  • Storybook

Thank you for pointing this out. This has been fixed now. You can check and verify with the storybook link in the pr. The problem occurred because the styleMap(style) directive was used to bind the dynamic styles to the element, but it didn't update the background gradient as expected. This happened because styleMap expects the style object to be properly structured, and its internal logic tracks changes to style properties that are directly applied as key-value pairs. If the style object was not correctly updated or its properties weren't tracked properly, the gradient was not updated as expected.

To resolve this, we switched to using the string interpolation method: style="background: ${style.background};". This approach directly applies the updated background value to the style attribute, bypassing the limitations of styleMap in this case.

Iโ€™ve updated the implementation to use string interpolation, and the gradient now updates correctly as expected.

Let me know if you have any questions or further concerns!

blunteshwar avatar Jan 08 '25 09:01 blunteshwar

Thank you, I verified that the fix looks good!

blaabaer avatar Jan 08 '25 21:01 blaabaer

Let's stage this for next release. Once this release is out we can make an additional announcement for this incoming change. @blunteshwar I will draft you a gist to post for the group! Thank you for bringing this to closure!

Rajdeepc avatar Jan 28 '25 09:01 Rajdeepc