-
-
Notifications
You must be signed in to change notification settings - Fork 73
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
Update blocklyworkspace from external events via new props #127
base: main
Are you sure you want to change the base?
Conversation
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 @hcientist! I'm a little confused by what the updateXml
and updateJson
options are doing - it looks like they're static strings which are checked for changes via the useEffect
hook's dependencies. I'm worried that it might be too easy for a user to accidentally cause these references to change by accident on every render.
I'm wondering if, instead, we could have useBlocklyWorkspace
return some callback functions that do the same thing - i.e., in addition to workspace, xml, and json, it could also return updateXml
and updateJson
functions that components could call to manually trigger updates. Would that change work for your use case?
@@ -57,7 +78,7 @@ const useBlocklyWorkspace = ({ | |||
json: object | null; | |||
} => { | |||
// onImportError replaces onImportXmlError | |||
// This is done for not breaking the signature until depreaction | |||
// This is done for not breaking the signature until deprecation |
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.
whoops, thanks for the spelling fix! ❤️
@nbudin thanks for all your work on this project and for considering my PR! I think anything is fine here and defer to your experience with With your proposal, my code that currently:
Would instead:
I am working to update the |
I don't think you'd necessarily need to change very much about your existing component (although I'd have to see it to say for sure). What I'm thinking is that if you wanted to track the JSON/XML as a piece of state in your component, you could do something like this: const { updateJson } = useBlocklyWorkspace(); // plus whatever else you're doing here
const [json, setJson] = useState('');
const onJsonChange = (event) => {
setJson(event.target.value);
updateJson(event.target.value);
};
return <textarea value={json} onChange={onJsonChange} />; Does that make sense? |
Ok, yes. Thanks for the clarification. In our use case we are using
So if it makes more sense for this project to have the props as callbacks passed via props to the hook, rather than as props on the component, that is totally fine. For our use case we need both the dom element and the |
updateXml
andupdateJson
, which if passed to the component/hook will result in the blockly workspace rerendering when the values of either of these new properties is changed. ( fixes add prop for updating blocks from external events #125 )README
anduseBlocklyWorkspace