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

Add a PlainColorObject type for functions that return color objects #291

Merged
merged 1 commit into from
Apr 12, 2023

Conversation

lloydk
Copy link
Collaborator

@lloydk lloydk commented Mar 1, 2023

The ColorObject type has optional properties and doesn't accurately reflect the color object type returned by functions such as getColor() which ensures that the object returned has space, coords and alpha properties that are not undefined.

A new interface called PlainColorObject has been introduced to represent fully formed color objects.

The Color class implements the PlainColorObject interface so the Color class was removed from list of types in the ColorTypes type.

This PR also fixes the return type for functions in the interpolation module that were returning the incorrect type.

@LeaVerou
Copy link
Member

LeaVerou commented Mar 1, 2023

Hey, thanks for the contribution! I've requested review from some more TS-savvy folks than me. Meanwhile, you could rebase this PR, as it now shows changes from your other, merged, PR.

@lloydk
Copy link
Collaborator Author

lloydk commented Mar 1, 2023

I managed to remove the extra commit from my original pull request but I think I ended up creating a different mess. I have no idea what I'm doing when it comes to git commands. For now I'll just wait for the review and if everything is ok we can figure out what to do next.

Copy link
Member

@jgerigmeyer jgerigmeyer left a comment

Choose a reason for hiding this comment

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

I like this! I think there may be a few more places where we can replace ColorObject with PlainColorObject (e.g. Display.color in display.d.ts). I haven't looked through all of them, but I expect there might be other places too?

@lloydk
Copy link
Collaborator Author

lloydk commented Mar 2, 2023

I like this! I think there may be a few more places where we can replace ColorObject with PlainColorObject (e.g. Display.color in display.d.ts). I haven't looked through all of them, but I expect there might be other places too?

I tried to find all of the functions that could be switched but it's quite possible I missed a few.

The display function calls serialize which calls getColor so in the case of the display function ColorTypes is the correct type for the color parameter.

@jgerigmeyer
Copy link
Member

The display function calls serialize which calls getColor so in the case of the display function ColorTypes is the correct type for the color parameter.

I meant the return value Display. Shouldn't it be defined as export type Display = string & { color: PlainColorObject };?

The ColorObject type has optional properties and doesn't
accurately reflect the color object type returned by
functions such as getColor() which ensures that the object
returned has space, coords and alpha properties that are
not undefined.

A new interface called PlainColorObject has been introduced
to represent fully formed color objects.

The Color class implements the PlainColorObject interface
so the Color class was removed from list of types in the
ColorTypes type.
@lloydk
Copy link
Collaborator Author

lloydk commented Mar 3, 2023

The display function calls serialize which calls getColor so in the case of the display function ColorTypes is the correct type for the color parameter.

I meant the return value Display. Shouldn't it be defined as export type Display = string & { color: PlainColorObject };?

The display function calls serialize which calls getColor so in the case of the display function ColorTypes is the correct type for the color parameter.

I meant the return value Display. Shouldn't it be defined as export type Display = string & { color: PlainColorObject };?

Sorry, misread your comment. You're correct and I've made the appropriate change to the Display type.

lloydk referenced this pull request Apr 11, 2023
The function represented by Range should return a Color, not a
number.

Fixes #298.
@LeaVerou
Copy link
Member

Just realized this PR had not been merged because it was referenced by another issue. Sorry @lloydk! FWIW @jgerigmeyer @MysteryBlokHed if I tag you for review, I fully trust you to merge the PR when/if ready. 😊

@LeaVerou LeaVerou merged commit b829d9f into color-js:main Apr 12, 2023
@lloydk lloydk deleted the plain-color-object-type branch February 22, 2024 08:20
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