feat(reactive-controllers): Migrate to Colorjs from Tinycolor
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.
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:
- Spectrum | Light | Medium | LTR
- Spectrum | Dark | Large | RTL
- Express | Light | Medium | LTR
- Express | Dark | Large | RTL
- Spectrum-two | Light | Medium | LTR
- Spectrum-two | Dark | Large | RTL
- High Contrast Mode | Medium | LTR
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.
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 | |
|---|---|
| Change from base Build 13496959333: | -0.001% |
| Covered Lines: | 33328 |
| Relevant Lines: | 33765 |
๐ - Coveralls
Tachometer results
Currently, no packages are changed by this PR...
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 |
@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?
@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.
Just a minor change or two in the docs and we're good to go.
Can you point out those changes in docs?
โ ๏ธ 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
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
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.
- 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!
Thank you, I verified that the fix looks good!
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!