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

Added missing constructors to ImageData facade. #829

Merged
merged 13 commits into from
Mar 21, 2024

Conversation

dragonfly-ai
Copy link
Contributor

The ImageData spec references several constructors that scala-js-dom has omitted:

https://developer.mozilla.org/en-US/docs/Web/API/ImageData/ImageData

This PR means to include these features.

@dragonfly-ai
Copy link
Contributor Author

dragonfly-ai commented Jan 12, 2024

Should we introduce an ImageSettings class?

The JS docs just supply them with anonymous arguments:

let imageData = new ImageData(200, 100, { colorSpace: "display-p3" });

Imported Uint8ClampedArray to reduce verbosity
@armanbilge
Copy link
Member

Should we introduce an ImageSettings class?

Yes. Here's an example of how it should be defined.

https://github.com/scala-js/scala-js-dom/blob/efe1b87772b114c95a3d30fcdddc603f0ebb0b23/dom/src/main/scala/org/scalajs/dom/DOMRectInit.scala

@dragonfly-ai
Copy link
Contributor Author

OK, I'll try to add that, too.

c added 2 commits January 12, 2024 11:03
Ran scalafmtAll to fix formatting issues.
Fixed typo in settings parameter in ImageData constructors.
@dragonfly-ai
Copy link
Contributor Author

dragonfly-ai commented Jan 12, 2024

What about adding a companion object to ImageSettings since its only field supports exactly two values: "srgb" and "display-p3"?

I mean to store static values for the two types of ImageSettings instances needed to encode the two supported color models.

@zetashift
Copy link
Contributor

What about adding a companion object to ImageSettings since its only field supports exactly two values: "srgb" and "display-p3"?

Wouldn't these be better off as an enum then? E.g. ImageSettingsColorSpace?
@dragonfly-ai This PR LGTM, if you want to add the enum or not, let me know, it can always be a separate ticket/PR

@dragonfly-ai
Copy link
Contributor Author

Thanks for approving!
I've never tried passing a Scala enum to a native JS function; do Scala enumerations interoperate well with plain JavaScript?

@sjrd
Copy link
Member

sjrd commented Mar 7, 2024

No, they don't.

@dragonfly-ai
Copy link
Contributor Author

Well, in that case, companion object with constants for display-p3 and srgb, or should we just leave it out?

Added ImageSettings Object with static values for sRGB and displayP3.
@dragonfly-ai
Copy link
Contributor Author

Added the ImageSettings object as discussed.

Copy link
Member

@armanbilge armanbilge left a comment

Choose a reason for hiding this comment

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

nice work, thanks!

@armanbilge armanbilge merged commit 53f9a1a into scala-js:main Mar 21, 2024
5 checks passed
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