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

Fix: change Color.set() override order to set ColorRepresentation function signature #614

Merged
merged 1 commit into from
Sep 25, 2023

Conversation

michealparks
Copy link
Contributor

Why

Since r153 Typescript will throw errors when setting the Color property in a Threlte application, because Threlte will use the Parameters of set() to determine types.

What

It was suggested in this three-ts-types discussion to change override order to fix this issue.

Checklist

  • Checked the target branch (current goes master, next goes dev)
  • Ready to be merged

@Methuselah96
Copy link
Contributor

Methuselah96 commented Sep 25, 2023

@michealparks Thanks for the PR!

Just to clarify, this would break the use-case of calling <T.MeshStandardMaterial color={[255, 0, 0]} />, correct? That may be the most pragmatic decision, given that I'm sure passing a string or Color is much more common, just want to make sure I'm understanding correctly that this does not completely solve the issue, it just solves the more common case.

@michealparks
Copy link
Contributor Author

michealparks commented Sep 25, 2023

Thank you for reviewing! You're correct. Although the issue won't be completely solved (as per your example) the following is much more common in Threlte:

<T.MeshStandardMaterial color='turquoise' />

<!-- or -->
<!-- color is an instance of THREE.Color -->
<T.MeshStandardMaterial color={color} />

@Methuselah96 Methuselah96 changed the base branch from master to dev September 25, 2023 21:56
Copy link
Contributor

@Methuselah96 Methuselah96 left a comment

Choose a reason for hiding this comment

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

Thanks! This will get released with r157 at the end of the month.

@Methuselah96 Methuselah96 merged commit 415f91e into three-types:dev Sep 25, 2023
3 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.

None yet

2 participants