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: make textinstance no-op instead of warn #3366

Merged
merged 1 commit into from
Oct 1, 2024

Conversation

Amatewasu
Copy link
Contributor

This PR aims to implement the behavior described in #3302

I did not achieve to test it on a local project (even though I succeeded a few months ago). Do you have some documentation to provide to compile and test on a project?

Copy link

codesandbox-ci bot commented Sep 24, 2024

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 213d2c3:

Sandbox Source
example Configuration

@DennisSmolek
Copy link
Contributor

I did not achieve to test it on a local project (even though I succeeded a few months ago). Do you have some documentation to provide to compile and test on a project?

You can run your changes against the examples by running yarn examples

Heres more info on building/testing: https://github.com/pmndrs/react-three-fiber/blob/master/CONTRIBUTING.md

@Amatewasu
Copy link
Contributor Author

@DennisSmolek Thank you a lot!

Printing the text definitely works (I've added a fragment: <>hello</>):

image

But I struggle to pass the property to the renderer. We might want to only print the text.

@CodyJasonBennett
Copy link
Member

Perhaps we should only no-op here. We can't make warnings or errors useful without tracing, and I have not found a reliable way to obtain this.

@Amatewasu
Copy link
Contributor Author

Amatewasu commented Sep 24, 2024

no-op would make sense because we do not expect text to be drawn without a proper component in a r3f canvas

if ok for you i can update the PR

@Amatewasu
Copy link
Contributor Author

@CodyJasonBennett I have just updated the PR to make handleTextInstance no-op (super light PR...)

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