Skip to content

Draft 1.1 changes for Color spaces JS API #3704

Merged
nex3 merged 15 commits intosass:mainfrom
oddbird:color-spaces-js-changes
Oct 3, 2023
Merged

Draft 1.1 changes for Color spaces JS API #3704
nex3 merged 15 commits intosass:mainfrom
oddbird:color-spaces-js-changes

Conversation

@jamesnw
Copy link
Contributor

@jamesnw jamesnw commented Sep 28, 2023

@jamesnw jamesnw marked this pull request as ready for review September 29, 2023 20:27
@jgerigmeyer
Copy link
Contributor

@nex3 Of course there may be more changes along the way, but this first set is probably ready for review/merge?

Comment on lines +412 to +417
* If any key in `keys` is not the name of a channel in `components`:

* If `initialSpace` is a [legacy color space] and `spaceSetExplicitly` is
false, emit a deprecation warning named `color-4-api`.

* Otherwise, throw an error.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can comfortably start throwing an error without updating the spec here and call it a potentially-breaking bug fix. We have a general rule that if users violate the TypeScript types, the behavior is undefined. We'll try to deprecate in cases where it's very likely that the types were being violated in good faith (like passing null instead of undefined to optional parameters), but I think it's pretty unlikely that anyone is actually running new SassColor({hue: ..., red: ...}) and expecting it to work.

Copy link
Contributor

Choose a reason for hiding this comment

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

Addressed in 7308783

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nex3 This brings a related point up, then. Currently the proposed types for change do allow for myColor.change({hue: ..., red: ..., x: ...}) through the overload without a space defined. We could limit the types by removing that overload, and then setting the space to optional in all the other types. I think this also communicates the interface a bit more directly as well.

* If `space` equals `lch` or `oklch`, let `changedColor` be the result of:

```js
SassColor({
Copy link
Contributor

Choose a reason for hiding this comment

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

It occurs to me that these should probably be

Suggested change
SassColor({
new SassColor({

Copy link
Contributor

Choose a reason for hiding this comment

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

Addressed in 7308783

@jgerigmeyer
Copy link
Contributor

@nex3 Ready for another round of review.

@nex3 nex3 merged commit 50feffd into sass:main Oct 3, 2023
@jgerigmeyer jgerigmeyer deleted the color-spaces-js-changes branch October 7, 2023 19:23
jgerigmeyer added a commit to oddbird/sass that referenced this pull request Oct 12, 2023
* main:
  Remove `isAlphaMissing` and add "alpha" to channel name types.
  Rename types using title-case for acronyms longer than two letters in camel-case identifiers.
  Clarifying performance expectations about sass and sass-embedded (sass#3716)
  Tweak language around getters and arrays.
  Code review
  Remove unused ValueObject type
  [First-Class Mixins] Flush to spec
  Move and restructure how we specify scopes
  Fix typos in the deprecation APi proposal (sass#3708)
  Return immutable types
  Remove generic change overload
  Add first class mixins to embedded sass protobuf and JS API (sass#3674)
  Draft 1.1 changes for Color spaces JS API  (sass#3704)
  Add support for the relative color syntax from CSS Color 5 (sass#3676)
  Bump tj-actions/changed-files from 39.0.3 to 39.2.0 (sass#3697)
  Package Importer updates (sass#3699)
  Fix link in Contributing docs (sass#3705)
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.

3 participants