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

feat(app): 421 Update components to use useLoop instead of useRenderLoop #422

Conversation

JaimeTorrealba
Copy link
Member

@JaimeTorrealba JaimeTorrealba commented May 16, 2024

close #421

Copy link

stackblitz bot commented May 16, 2024

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@JaimeTorrealba JaimeTorrealba marked this pull request as draft May 16, 2024 13:43
@JaimeTorrealba
Copy link
Member Author

Hi, currently 3 main things are missing for this one

@alvarosabu could you check please the lint problems with ocean (a problem with the demo that uses SKY) and holographic

@andretchen0 for some reason, Sparkles is not updating can you help me with this one

Contact shadows also have problems, but this comes before.

And useEnviroment needs to be checked (I will do this)

Thanks :)

@alvarosabu
Copy link
Member

Sure @JaimeTorrealba, I already fixed the issue of the linting for Sky and holographic here #424, just checked Ocean and is solved as well, once that is merge you can merge it here to do the update

For contact shadows the issues are different, it will require its own ticket

@andretchen0
Copy link
Contributor

Unsure about Sparkles. Maybe we take it offline for now so this can move forward.

I checked it after the linter was updated.

I'm guessing an underlying dependency updated, but I can't find the source atm.

@JaimeTorrealba
Copy link
Member Author

Consider that the ContactShadows and the sparkles will be out of the scope of this PR.

Can we merge this one? @alvarosabu @andretchen0

@alvarosabu
Copy link
Member

Consider that the ContactShadows and the sparkles will be out of the scope of this PR.

Can we merge this one? @alvarosabu @andretchen0

If we take out those 2 yes, I will take over the ContactShadows 👌

@JaimeTorrealba JaimeTorrealba marked this pull request as ready for review May 21, 2024 14:25
@andretchen0
Copy link
Contributor

andretchen0 commented May 25, 2024

As mentioned on Discord, it seems that the problem with Sparkles and ContactShadows are due to changes in the way primitive is handled by the v4 core.

Considering the amount work that @JaimeTorrealba has done here, I think it'd be best to merge now and recheck after fixing primitives in the core. But I'll leave that decision up to you two, @alvarosabu @JaimeTorrealba .

@alvarosabu
Copy link
Member

I agree with @andretchen0, please @JaimeTorrealba check the conflict on the package.json and we are ready to merge

@JaimeTorrealba JaimeTorrealba merged commit f8c0619 into v4 May 27, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

3 participants