Add code input for number props #364
Conversation
|
@benjamincloquet is attempting to deploy a commit to the Triplex Team on Vercel. A member of the Team first needs to authorize it. |
|
Awesome mate I'll take a look asap :-) — feel free to make comments over the PR if you think there's areas of interest! |
itsdouges
left a comment
itsdouges
left a comment
There was a problem hiding this comment.
heya, pulled it down and had a look. good job! impressed you were able to work your way through the codebase, any major troubles? here's some notes:
- it works! cool
- the hover state still shows the
cursor-col-resize— since dragging to change the number isn't available we should remove this when in expression mode - you have some failing tests :-)
- make sure to add a changeset
pnpm changesetthat describes the new feature
| actionId="contextpanel_input_number_expression_toggle" | ||
| icon={CodeIcon} | ||
| label={ | ||
| mode === "number" |
There was a problem hiding this comment.
any thoughts on how we could have this auto infer? e.g. when typing into the input it would change to expression mode if an expression is found (or alternatively/more simply, maybe it is always in expression mode for number inputs?) and then we can enable / disable features if the value is an expression vs. number literal (the latter enables dragging to change the value)
There was a problem hiding this comment.
Ohh that's smart, I like the idea of having the input always be in expression mode since a number is an expression albeit a simple one. I think I'll have a second go at it cause this all could be simpler than having a toggle when I think about it. thx!
| return { kind: "number", value: Number(expression.getLiteralText()) }; | ||
| } | ||
|
|
||
| if (expression && evaluateNumericalExpression(expression.getText())) { |
There was a problem hiding this comment.
ah cool so if we can evaluate it then we allow it, nice
| const project = _createProject({ | ||
| tsConfigFilePath: join(__dirname, "__mocks__/tsconfig.json"), | ||
| }); | ||
| const sourceFile = project.addSourceFileAtPath( |
There was a problem hiding this comment.
nit: you can use createSourceFile and have it embedded in the test instead of having it in the mocks folder
tbh I would have done that but didn't realize it existed until much later
| */ | ||
| export function evaluateNumericalExpression(expression: string): number | null { | ||
| try { | ||
| const func = new Function(`return ${expression}`); |
There was a problem hiding this comment.
any thoughts on sanitizing this input
There was a problem hiding this comment.
Oh yep that's definitely a good idea
| } | ||
| }), | ||
| on("request-set-element-prop", (data) => { | ||
| const propValue = unwrapPropValue(data.propValue); |
There was a problem hiding this comment.
ah because we receive the prop as the expression literal not the string 🤔
and this is only for setting the temporary value before it's persisted?
what if we updated this code path to always take the evaluated value and then for the actual prop setting it takes the serializable prop values / expressions?
before if I remember correctly it just assumes everything is one in the same, now it can be different
There was a problem hiding this comment.
Good idea, I'll look into it :)
| return; | ||
| } | ||
|
|
||
| // For saving, wrap the expression in a RawCodeExpression because we don't want it to be interpreted as a regular string |
There was a problem hiding this comment.
makes sense. and then because of this we've updated the use-temporary-props logic to handle it
perhaps confirm should take two args, one as the evaluated value, and the second as an optional expression? then we handle that in the code as appropriate
could be the start of a properly defined system and we can type it more easily as well
There was a problem hiding this comment.
Ah that's smart, it would definitely make the code more readable in the future. on it !
|
@itsdouges hey Mike, back on this as I was away for the last weeks - I have an idea to improve this without needing a whole new component, just making the NumberInput smarter so that we don't need a manual toggle 👍 quick question, what's your preferred setup when it comes to logging debug info? I'm very new to extension development and I'm struggling to do my basic console logs. I somehow managed when I first jumped on this but I can't get it to work anymore 😭 any tips? |
|
Console logs and viewing the developer console when building :-) Open it with command palette! |
2 participants
Experimenting on this issue #254
This PR aims to improve the regular number input (as seen for the
position,rotation,scaleprops) by adding a toggle to allow the user to input simple code expressions.In this context, a valid expression is an expression that can be resolved in isolation, eg.
2 + 2,Math.PI / 2,Math.sqrt(2)and so on. If a prop contains an identifier from an external variable (myVariable * 2), it then falls under the standard unhandled category.Recommended review path:
NumberInputand aStringInputto allow switching between number and code inputs.undefinedto end up as a string in the code when saving, we want to save actual expressions in the code (Math.PIinstead of"Math.PI")To be improved : there is no visible validation as of now, if an incorrect expression is saved, the prop value is silently replaced with
undefined