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

[Color 4] Add support for CSS Color Level 4. #259

Merged
merged 28 commits into from
Nov 17, 2023

Conversation

@jgerigmeyer jgerigmeyer requested a review from nex3 October 19, 2023 20:29
lib/src/value/color.ts Outdated Show resolved Hide resolved
lib/src/value/color.ts Outdated Show resolved Hide resolved
lib/src/value/color.ts Outdated Show resolved Hide resolved
lib/src/value/color.ts Outdated Show resolved Hide resolved
@ntkme
Copy link
Contributor

ntkme commented Oct 20, 2023

This faces a challenge like Calculation simplification, which we decided to not implement on the host side. Currently what I see from this PR is that we would need to implement the same feature twice, once in dart for dart-sass, and then once in pure js for this project. I think we should consider adding Color.change feature to embedded protocol, that is to let the host send an existing color and arguments to change, and then the embedded compiler returns the color after change. This way we don’t need implement the same logic twice or worry about potential drifts in different implementation.

@nex3
Copy link
Contributor

nex3 commented Oct 23, 2023

@ntkme I'm generally wary about asking embedded hosts to make round trips with the compiler just to do value operations, because it opens up a lot of difficulties both in terms of the API design (especially in languages with a strict distinction between synchrony and asynchrony) and in terms of performance (I'd like to avoid hidden cliffs where an operation that looks like it could be efficient is actually very slow). I think maybe the best solution here would just be to say that we don't mandate embedded hosts support all the color operations that the JS API does.

* main:
  Update Dart Sass version and release
lib/src/value/color.ts Outdated Show resolved Hide resolved
lib/src/value/color.ts Outdated Show resolved Hide resolved
lib/src/value/color.ts Outdated Show resolved Hide resolved
lib/src/value/color.ts Outdated Show resolved Hide resolved
lib/src/value/color.ts Outdated Show resolved Hide resolved
lib/src/value/color.ts Outdated Show resolved Hide resolved
@jgerigmeyer jgerigmeyer requested a review from nex3 November 2, 2023 22:33
@jgerigmeyer
Copy link
Contributor Author

@nex3 I think this could use another round of quick review -- thanks!

Copy link
Contributor

@nex3 nex3 left a comment

Choose a reason for hiding this comment

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

Looks good at a quick glance. Remember to document all functions and include return types before the final.

lib/src/value/color.ts Outdated Show resolved Hide resolved
lib/src/value/color.ts Outdated Show resolved Hide resolved
@jgerigmeyer jgerigmeyer marked this pull request as ready for review November 10, 2023 03:06
@jgerigmeyer
Copy link
Contributor Author

jgerigmeyer commented Nov 10, 2023

@nex3 I think this is ready for another round of review. AFAICT all of the API is implemented. I can't reproduce the failing tests locally -- something to do with the legacy color definition in the embedded host? CI is passing.

lib/src/value/utils.ts Outdated Show resolved Hide resolved
lib/src/value/color.ts Outdated Show resolved Hide resolved
lib/src/value/color.ts Show resolved Hide resolved
lib/src/value/color.ts Outdated Show resolved Hide resolved
lib/src/value/color.ts Outdated Show resolved Hide resolved
lib/src/value/color.ts Outdated Show resolved Hide resolved
lib/src/value/color.ts Outdated Show resolved Hide resolved
lib/src/value/color.ts Outdated Show resolved Hide resolved
lib/src/value/color.ts Outdated Show resolved Hide resolved
lib/src/value/color.ts Outdated Show resolved Hide resolved
@jgerigmeyer
Copy link
Contributor Author

@nex3 Thanks for the review -- I think I've addressed everything, and this is ready for another round!

@nex3
Copy link
Contributor

nex3 commented Nov 16, 2023

Can you rebase this onto the feature.color-4 branch?

* feature.color-4:
  Make compiler-version -dev
  Poke CI
  Poke CI
  Run tests on pushes to feature branches (sass#171)
  Fetch matching feature branches (sass#169)
@jgerigmeyer jgerigmeyer changed the base branch from main to feature.color-4 November 16, 2023 03:09
@jgerigmeyer
Copy link
Contributor Author

Can you rebase this onto the feature.color-4 branch?

@nex3 Done, though I had already been basing this on main and it looks like feature.color-4 is behind main, so there are some additional changes here until main is merged into feature.color-4.

@nex3 nex3 merged commit 8c82e53 into sass:feature.color-4 Nov 17, 2023
12 checks passed
@jgerigmeyer jgerigmeyer deleted the js-api-color-4 branch November 18, 2023 02:55
jgerigmeyer referenced this pull request in color-js/color.js Feb 6, 2024
Add short description + impact
@jgerigmeyer I recall you work on some high impact projects that depend on Color.js, can you add a few examples in the third bullet?
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.

4 participants