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

refactor: remove TweakPane, use Leches #308

Merged

Conversation

andretchen0
Copy link
Contributor

@andretchen0 andretchen0 commented Dec 11, 2023

Removes TweakPane in demos. Uses Leches instead.

Related #183

@andretchen0 andretchen0 linked an issue Dec 11, 2023 that may be closed by this pull request
14 tasks
Copy link

stackblitz bot commented Dec 11, 2023

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

Copy link

netlify bot commented Dec 11, 2023

Deploy Preview for cientos-tresjs ready!

Name Link
🔨 Latest commit 8720178
🔍 Latest deploy log https://app.netlify.com/sites/cientos-tresjs/deploys/65800db84747130008f97076
😎 Deploy Preview https://deploy-preview-308--cientos-tresjs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@andretchen0 andretchen0 marked this pull request as ready for review December 11, 2023 15:32
@andretchen0 andretchen0 changed the title refactor: FBODemo - remove TweakPane, use Leches refactor: remove TweakPane, use Leches Dec 11, 2023
@andretchen0 andretchen0 removed the request for review from alvarosabu December 11, 2023 22:44
@alvarosabu
Copy link
Member

Hi @andretchen0 thanks for the PR, two questions:

  • Would you create a PR for each refactor or should we mark this as a Draft?
  • How do you feel using TresLeches vs Tweakpane, does it improve the DX or make it difficult? TresLeches is still on early alpha I would love to know how could we improve it

@alvarosabu alvarosabu added the v4 label Dec 12, 2023
@andretchen0 andretchen0 marked this pull request as draft December 12, 2023 12:16
@andretchen0
Copy link
Contributor Author

andretchen0 commented Dec 12, 2023

@alvarosabu

Converted to a draft. If you'd like me to split the PR by file, let me know. No problem either way.

Fwiw, I'm almost done with CameraControls. Continuing at this pace, even if no one else pitches in, all the files from the "migrate" list should be ready for review by the end of the week, if that changes how you'd like to proceed.


How do you feel using TresLeches vs Tweakpane, does it improve the DX or make it difficult? TresLeches is still on early alpha I would love to know how could we improve it

For functionality, I think it's generally equivalent. What you can do in one, you can largely do in the other.

For improving Leches further, these things come to mind.

Compact view

For the same controls, Tweakpane is roughly half the height of Leches. I often find myself tweaking/removing controls to get Leches to fit in a window. That's a design tradeoff, but since I'm mostly working on docs/playground examples aimed at other devs, I'd trade whitespace/larger controls/larger type for having more controls "above the fold".

Embeddable in a page

For the Cientos docs examples, it'd be great if we could embed the controls above/below the inline THREE window. I often find myself removing controls I'd like to show off because there's just not enough space in the little inline demo window for another Leches widget.

Folder API

I often have to consult the source/tests to figure out how to add folders – i.e., useControls("folder name", controls).

Afaik, for multiple folders, you have to make multiple calls to useControls – e.g.:

useControls("folder 1", controls1); 
useControls("folder 2", controls2);

Maybe using the usual API and adding a folder key would be more straightforward? E.g.:

useControls({
  'reset': {
    folder: 'folder 1',
    ...
  }, 
  'increment': {
    folder: 'folder 2',
    ...
  }})

value.value

If you need to watch the value of a widget that gets returned from useControls you have to watch([widget.value, ...] ... and use widget.value.value. This often trips me up.

Examples

I often find myself reading the Leches source/tests to figure out how to do a simple thing. I'd like that to be easier/faster. A short page of screenshots and code would help, I think.

Maybe it could be added to the README if it's not too long.

Copy link
Member

@andretchen0 I can't thank you enough for the detailed feedback, I will work on improving tres leches based on what you provided me

@andretchen0
Copy link
Contributor Author

andretchen0 commented Dec 15, 2023

@alvarosabu

Following the conversation at #183, I've removed all Tweakpane references from the docs site.

Just fyi ...

Miscellaneous removed references to Tweakpane

Aside from imports of useTweakpane, the following were also removed:

  • dependency from docs/package.json
  • docs page for useTweakpane
  • link to useTweakpane doc page from the docs site sidebar

Leches version bump in playground

In the playground/CameraControlsDemo there were buttons in the Tweakpane UI. Leches has buttons as of v0.14, but cientos was using v0.13.

I bumped the Leches version only in playground/package.json.

I didn't bump the Leches version anywhere else.

@andretchen0 andretchen0 marked this pull request as ready for review December 15, 2023 18:18
Copy link
Member

Hi @andretchen0 I think we should keep the docs pages but with a warning that has been deprecated since version X and adds a link to v-tweakpane as an option for users, wdyt?

@andretchen0
Copy link
Contributor Author

@alvarosabu

Ok, I've reverted the deletion of the useTweakpane docs page and added the link back to the docs sidebar nav.

but with a warning that has been deprecated since version X

Sure, I added a "deprecated" badge.

On the docs site and in the module itself, there's been a "soon to be deprecated" warning for a while. I said it was deprecated "as of v3.7.0" – the current version of Cientos. Is that ok?

and adds a link to v-tweakpane as an option for users, wdyt?

Sure thing. Suggestions to migrate to Leches or v-tweakpane have been up for a while. I left those in place.

Copy link
Member

@alvarosabu alvarosabu left a comment

Choose a reason for hiding this comment

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

Incredible amount of work, thanks a lot @andretchen0 🙏🏻

@alvarosabu alvarosabu merged commit b636043 into main Dec 18, 2023
6 checks passed
@andretchen0 andretchen0 deleted the bugfix/183-start-migrating-playground-controls-to-tresjsleches branch December 18, 2023 15:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Start migrating playground controls to @tresjs/leches
2 participants