-
-
Notifications
You must be signed in to change notification settings - Fork 40
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
refactor(app)!: 160 Replace value.value for value.root #350
Conversation
Run & review this pull request in StackBlitz Codeflow. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @JaimeTorrealba ,
It looks like this is the result of a find + replace. Unfortunately, that won't work here because at least Leches is still using value.value
. Leches is used in a lot of demos and in the docs.
See:
pnpm run playground
... then visit ...
@@ -144,106 +144,106 @@ const [seedRef, scaleRef] = useControls( | |||
seedPropsRef.value = [ | |||
{ | |||
texture: [line, ring], | |||
color: [oversizeColorA.value.value, oversizeColorB.value.value, oversizeColorC.value.value], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @JaimeTorrealba ,
This change and others like it cause an error to be thrown or (worse) they fail silently. (That's JS for you! 😆)
oversizeColorA
comes from Leches, not Cientos. Leches is still using value.value
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I Couldn't try the lensflare, is completely broken, and you're right I use find and replace.
But the lensflares is broken from the main branch. In any case, let me go back to this changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I almost never use leches I didn't tested well, thanks for point this one :)
I'll be happy to check and modify whatever components/demos I've worked on. (And others too.) Since this PR seems to have gotten off on the wrong foot, maybe let's close and start a new PR? Also, since this is going to require reading through the files, to organize the effort, maybe it would be handy to create a markdown list in the top comment of the new PR with something like:
Then we can all commit to the same branch and check off as we go? |
Do you prefer to take the leadership here? @andretchen0 |
Hi, should I continue to work on this one? Or would you take it @andretchen0 |
Hey @JaimeTorrealba Sorry, I didn't see the earlier message. I can take this if you like. I'll hop on it when I finish the new component I am working on. |
@andretchen0 perfect, and thanks. Feel free to close this branch/PR if you want |
Hey @JaimeTorrealba @alvarosabu I'm working on this currently. In making the changes, I started wondering about What about
instead of
Would |
Yep, I like instance more too. |
I agree as well |
Questions for you guys, some of these components doesn't export anything, should they (do you consider is a good idea)
If so, let me know, I can do it without any problem
Also some demos are broken, but this as nothing to do with the current change.
EJ.
Lensflare (this is for leches)
enviroment (don't know what is the problem here) (this one export the instance correctly)