h3n3/cadhub
1
0
Fork 0
Code Issues 35 Pull Requests Packages Projects Releases Wiki Activity

CadHub Customizer #461

Merged
Irev-Dev merged 1 commits from kurt/cadhub-customizer-test-code-437 into main 2021-08-21 03:02:56 +02:00
Conversation 12 Commits 1 Files Changed 9 +526 -414
Irev-Dev commented 2021-08-20 14:11:09 +02:00 (Migrated from github.com)
Copy Link

Start of implementation of the CadHub customizer

follows https://github.com/Irev-Dev/cadhub/discussions/437

The idea is there is a consistent customizer on Cadhub that will work with multiple integrations.

So far this implements sting number and boolean for JSCAD, (and should work with OpenSCAD but that work is ongoing see: https://github.com/openscad/openscad/issues/3801)

It would be good to get select and groups working before we release the JSCAD integration, but I'm okay if that's added later.

Start of implementation of the CadHub customizer follows https://github.com/Irev-Dev/cadhub/discussions/437 The idea is there is a consistent customizer on Cadhub that will work with multiple integrations. So far this implements sting number and boolean for JSCAD, (and should work with OpenSCAD but that work is ongoing see: https://github.com/openscad/openscad/issues/3801) It would be good to get select and groups working before we release the JSCAD integration, but I'm okay if that's added later.
franknoirot (Migrated from github.com) reviewed 2021-08-20 14:11:09 +02:00
Irev-Dev (Migrated from github.com) reviewed 2021-08-20 14:12:51 +02:00
app/web/src/components/Customizer/customizerConverter.ts
@@ -0,0 +32,4 @@
| CadhubNumberParam
// OpenSCAD
const openscadValues = `
Irev-Dev (Migrated from github.com) commented 2021-08-20 14:12:51 +02:00
Copy Link

The following is just here to test implementation for OpenSCAD and will be removed later.

The following is just here to test implementation for OpenSCAD and will be removed later.
Irev-Dev (Migrated from github.com) reviewed 2021-08-20 14:20:33 +02:00
app/web/src/helpers/cadPackages/jsCad/jsCadController.ts
@@ -81,0 +105,4 @@
script: code,
params: parameters || {},
url: 'jscad_script',
})
Irev-Dev (Migrated from github.com) commented 2021-08-20 14:20:33 +02:00
Copy Link

fundamentally i haven't changed how your code works, I just wanted to group things a little better in this file as I found some of the module level variables hard to keep track of and so instead moved some of the stuff that required state into this class. Plus one other thing that made me uncomfortable was how the function awaited a promise just to return another module level variable response.

This render function now sets up promise -> posts a message to the worker -> then returns the promise where the promise now contains the final response. I at least personally find it easier to reason about.

fundamentally i haven't changed how your code works, I just wanted to group things a little better in this file as I found some of the module level variables hard to keep track of and so instead moved some of the stuff that required state into this class. Plus one other thing that made me uncomfortable was how the function awaited a promise just to return another module level variable `response`. This render function now sets up promise -> posts a message to the worker -> then returns the promise where the promise now contains the final response. I at least personally find it easier to reason about.
Irev-Dev (Migrated from github.com) reviewed 2021-08-20 14:21:47 +02:00
app/web/src/helpers/cadPackages/jsCad/jsCadController.ts
Irev-Dev (Migrated from github.com) commented 2021-08-20 14:21:47 +02:00
Copy Link

Just passing the response into the callResolve equivalent so that the promise has the response inside.

Just passing the response into the `callResolve` equivalent so that the promise has the response inside.
Irev-Dev (Migrated from github.com) reviewed 2021-08-20 14:22:43 +02:00
app/web/src/helpers/cadPackages/jsCad/jsCadController.ts
Irev-Dev (Migrated from github.com) commented 2021-08-20 14:22:43 +02:00
Copy Link

This is what I mean from this comment https://github.com/Irev-Dev/cadhub/pull/461/files#r692901725 about awaiting a promise just to return a different variable.

This is what I mean from this comment https://github.com/Irev-Dev/cadhub/pull/461/files#r692901725 about awaiting a promise just to return a different variable.
hrgdavor (Migrated from github.com) reviewed 2021-08-20 15:41:41 +02:00
app/web/src/helpers/cadPackages/jsCad/jsCadController.ts
@@ -81,0 +105,4 @@
script: code,
params: parameters || {},
url: 'jscad_script',
})
hrgdavor (Migrated from github.com) commented 2021-08-20 15:41:41 +02:00
Copy Link

yes, it looks cleaner. thnx :)

yes, it looks cleaner. thnx :)
hrgdavor (Migrated from github.com) reviewed 2021-08-20 15:43:04 +02:00
app/web/src/helpers/cadPackages/jsCad/jsCadController.ts
hrgdavor (Migrated from github.com) commented 2021-08-20 15:43:04 +02:00
Copy Link

yeah, that was messy. thanks

yeah, that was messy. thanks
hrgdavor (Migrated from github.com) requested changes 2021-08-20 15:49:01 +02:00
hrgdavor (Migrated from github.com) left a comment
Copy Link

One issue I am having is that step parameter is not respected. I tried to trace it a bit, but I only found it gets lost and is not present as value in the event handlers. param.step is undefined.

also I think the logic for step will need updating to handle decimal steps.

there is a leftover console.trace in my jscadcontroller, please remove iunless you need it still.

I have tried pointer events, I they work in desktop env, but both your version with pointer lock and pointerevents don't work in mobile emulation for me in chrome.

I actually prefer the pointer lock functionality here, as it leaves the cursor in place.

One issue I am having is that step parameter is not respected. I tried to trace it a bit, but I only found it gets lost and is not present as value in the event handlers. param.step is undefined. also I think the logic for step will need updating to handle decimal steps. there is a leftover `console.trace` in my jscadcontroller, please remove iunless you need it still. I have tried pointer events, I they work in desktop env, but both your version with pointer lock and pointerevents don't work in mobile emulation for me in chrome. I actually prefer the pointer lock functionality here, as it leaves the cursor in place.
hrgdavor commented 2021-08-21 01:26:02 +02:00 (Migrated from github.com)
Copy Link

step is now respected. :)

one minor issue though, step is not respected while dragging ... that is irellevant now, but it may be relevant when/if u implement live update of params option.

step is now respected. :) one minor issue though, step is not respected while dragging ... that is irellevant now, but it may be relevant when/if u implement live update of params option.
hrgdavor (Migrated from github.com) approved these changes 2021-08-21 01:26:51 +02:00
hrgdavor (Migrated from github.com) left a comment
Copy Link

looks good aside from minor issue of step not in the preview while dragging slider

looks good aside from minor issue of step not in the preview while dragging slider
Irev-Dev commented 2021-08-21 03:11:12 +02:00 (Migrated from github.com)
Copy Link

step is now respected. :)

one minor issue though, step is not respected while dragging ... that is irellevant now, but it may be relevant when/if u implement live update of params option.

I'm not sure the steps should be enforced while the input is being actively dragged, to me it's disrespecting the user by interrupting them to change the input, rather that waiting until they are done to fix the input to match the steps.

I regards to live, I've been having trouble with it. You can see my attempt here https://github.com/Irev-Dev/cadhub/pull/new/kurt/live-customizer-attempt but something's not right, it doesn't always update when it should.

> step is now respected. :) > > one minor issue though, step is not respected while dragging ... that is irellevant now, but it may be relevant when/if u implement live update of params option. I'm not sure the steps should be enforced while the input is being actively dragged, to me it's disrespecting the user by interrupting them to change the input, rather that waiting until they are done to fix the input to match the steps. I regards to live, I've been having trouble with it. You can see my attempt here https://github.com/Irev-Dev/cadhub/pull/new/kurt/live-customizer-attempt but something's not right, it doesn't always update when it should.
Irev-Dev commented 2021-08-21 03:17:24 +02:00 (Migrated from github.com)
Copy Link

0cc335e is a rebase on main before merging.

0cc335e is a rebase on main before merging.
Sign in to join this conversation.
Reviewers
No Reviewers
franknoirot
Irev-Dev
hrgdavor
Labels
No items
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
h3n3
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: h3n3/cadhub#461
Delete Branch "kurt/cadhub-customizer-test-code-437"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?