volto icon indicating copy to clipboard operation
volto copied to clipboard

Fix internalUrl Widget to Reflect Prop Changes via onChangeBlock

Open dobri1408 opened this issue 1 year ago β€’ 1 comments

When you update a block's properties using onChangeBlock, the widget may not show these changes because its state isn't updating correctly.

https://github.com/plone/volto/assets/50819975/8243fe43-2531-49c4-8405-49f9d998666b

The expected behavior is that the URL field in the sidebar will update with the new URL value, as it was updated by onChangeBlock.


πŸ“š Documentation preview πŸ“š: https://volto--6036.org.readthedocs.build/

dobri1408 avatar May 21 '24 08:05 dobri1408

Deploy Preview for plone-components ready!

Name Link
Latest commit a230cb87f25052ca4f1f79649db49073c06623bb
Latest deploy log https://app.netlify.com/sites/plone-components/deploys/66695ecf6d8a9e00088432b5
Deploy Preview https://deploy-preview-6036--plone-components.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.

netlify[bot] avatar May 21 '24 08:05 netlify[bot]

I've set up a test using coresandbox to demonstrate the bug I'm referring to.

dobri1408 avatar May 24 '24 05:05 dobri1408

Sorry for the mistake, I have put the right name back.

dobri1408 avatar May 24 '24 18:05 dobri1408

@davisagli I've modified the logic to take the value from the prop as you've suggested. It was made this way because this widget is basically a copy of the URLWidget made by redturtle to have internal links which was done by Edw as part of the content rules and is only used on the content rules. Still some cleanup todo but the issue described here would affect also the original URL widget.

One change that I am pushing for is to avoid having undefined value when we clear because React complains about it about controlled vs non controlled inputs. Having an empty string as value keeps the return and input of the url as a string and it keeps the state in sync. have a look at this video to see how it behaves when we use undefined onClear vs an empty string.

This video shows the behavior in action https://github.com/plone/volto/assets/152852/29dacd78-3d49-4b87-9a6c-8dcdcb710bd6

ichim-david avatar May 31 '24 10:05 ichim-david

@ichim-david I think that's probably okay since an empty string evaluates to boolean False in Javascript -- as long as the backend accepts it when configuring a content rule. The other option would be setting it to null

davisagli avatar Jun 12 '24 05:06 davisagli

@ichim-david I think that's probably okay since an empty string evaluates to boolean False in Javascript -- as long as the backend accepts it when configuring a content rule. The other option would be setting it to null

@davisagli you can't You cannot pass value={undefined} first and later pass value="some string" because React won’t know whether you want the component to be uncontrolled or controlled. A controlled component should always receive a string value, not null or undefined. https://react.dev/reference/react-dom/components/input#im-getting-an-error-a-component-is-changing-an-uncontrolled-input-to-be-controlled

We get warnings and it doesn't work well if we have null or undefined. This is why I propose the empty string since it evaluates to false and it keeps the value in sync without the need for the use effect

ichim-david avatar Jun 12 '24 05:06 ichim-david