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

Fix internalUrl Widget to Reflect Prop Changes via onChangeBlock #6036

Merged
merged 33 commits into from
Jun 12, 2024

Conversation

dobri1408
Copy link
Contributor

@dobri1408 dobri1408 commented May 21, 2024

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

screen-capture.5.mp4

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/

Copy link

netlify bot commented May 21, 2024

Deploy Preview for plone-components ready!

Name Link
🔨 Latest commit a230cb8
🔍 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.

@dobri1408
Copy link
Contributor Author

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

@dobri1408 dobri1408 requested a review from sneridagh May 24, 2024 08:14
Copy link
Member

@ichim-david ichim-david left a comment

Choose a reason for hiding this comment

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

@dobri1408 you've renamed TestBlock Edit.jsx component InputBlockEdit and your InputBlock/View.tsx has name TestBlockView, put the naming correctly for your given new block and don't modify the TestBlock

@dobri1408
Copy link
Contributor Author

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

Copy link
Member

@davisagli davisagli left a comment

Choose a reason for hiding this comment

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

Would it be simpler to make this widget a controlled component (i.e. stop managing its own value as internal state and always show the value from props) instead of trying to sync changes from the value prop to the widget state?

- Also removed setting of undefined on clearing opting to set value to
  empty string to avoid warning that we switch from controlled to
  uncontrolled component.
@ichim-david
Copy link
Member

@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

@davisagli
Copy link
Member

@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

@ichim-david
Copy link
Member

@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

Copy link
Member

@davisagli davisagli left a comment

Choose a reason for hiding this comment

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

@ichim-david The empty string works fine where this is used -- and in the context where it is used for content rules, the empty value will never be sent to the backend, since the field is required.

@davisagli davisagli merged commit a2b42ff into main Jun 12, 2024
70 checks passed
@davisagli davisagli deleted the fix-internal-url-widget branch June 12, 2024 18:03
sneridagh added a commit that referenced this pull request Jun 15, 2024
* main: (63 commits)
  Release 18.0.0-alpha.35
  Release @plone/types 1.0.0-alpha.16
  (fix): export getFieldURL from Url.js in helpers (#6100)
  Improve container detection, `config.settings.containerBlockTypes` is no longer needed (#6099)
  Support nested directories in public folder add-on sync folders both … (#6098)
  Release 18.0.0-alpha.34
  Add generated new declarations for a34, just in case (#6097)
  Release @plone/slate 18.0.0-alpha.13
  Release @plone/registry 1.6.0
  Release @plone/types 1.0.0-alpha.15
  Add support for reading the add-ons `tsconfig.json` paths (#6096)
  Fixes #6046 pass proper defaults to align and size fields of Image block (#6093)
  bug fix. relations control panel. Restrict eglible relation targets a… (#6092)
  Fix Uncaught RangeError: date value is not finite in DateTimeFormat.format (#6088)
  Add optional `token` parameter to ploneClient initialization (#6077)
  Blocks Layout Navigator (#5642)
  Fix internalUrl  Widget to Reflect Prop Changes via onChangeBlock (#6036)
  [types] Improve styleClassNameExtenders types, Icon component JSDoc t… (#6095)
  Fix link in pop-up RelationsMatrix.jsx (#6085)
  Fix deselecting lists. (#6080)
  ...
sneridagh added a commit to aryan7081/volto that referenced this pull request Jun 15, 2024
* main: (42 commits)
  Release 18.0.0-alpha.35
  Release @plone/types 1.0.0-alpha.16
  (fix): export getFieldURL from Url.js in helpers (plone#6100)
  Improve container detection, `config.settings.containerBlockTypes` is no longer needed (plone#6099)
  Support nested directories in public folder add-on sync folders both … (plone#6098)
  Release 18.0.0-alpha.34
  Add generated new declarations for a34, just in case (plone#6097)
  Release @plone/slate 18.0.0-alpha.13
  Release @plone/registry 1.6.0
  Release @plone/types 1.0.0-alpha.15
  Add support for reading the add-ons `tsconfig.json` paths (plone#6096)
  Fixes plone#6046 pass proper defaults to align and size fields of Image block (plone#6093)
  bug fix. relations control panel. Restrict eglible relation targets a… (plone#6092)
  Fix Uncaught RangeError: date value is not finite in DateTimeFormat.format (plone#6088)
  Add optional `token` parameter to ploneClient initialization (plone#6077)
  Blocks Layout Navigator (plone#5642)
  Fix internalUrl  Widget to Reflect Prop Changes via onChangeBlock (plone#6036)
  [types] Improve styleClassNameExtenders types, Icon component JSDoc t… (plone#6095)
  Fix link in pop-up RelationsMatrix.jsx (plone#6085)
  Fix deselecting lists. (plone#6080)
  ...
sneridagh added a commit that referenced this pull request Jun 17, 2024
* main: (31 commits)
  Release 18.0.0-alpha.35
  Release @plone/types 1.0.0-alpha.16
  (fix): export getFieldURL from Url.js in helpers (#6100)
  Improve container detection, `config.settings.containerBlockTypes` is no longer needed (#6099)
  Support nested directories in public folder add-on sync folders both … (#6098)
  Release 18.0.0-alpha.34
  Add generated new declarations for a34, just in case (#6097)
  Release @plone/slate 18.0.0-alpha.13
  Release @plone/registry 1.6.0
  Release @plone/types 1.0.0-alpha.15
  Add support for reading the add-ons `tsconfig.json` paths (#6096)
  Fixes #6046 pass proper defaults to align and size fields of Image block (#6093)
  bug fix. relations control panel. Restrict eglible relation targets a… (#6092)
  Fix Uncaught RangeError: date value is not finite in DateTimeFormat.format (#6088)
  Add optional `token` parameter to ploneClient initialization (#6077)
  Blocks Layout Navigator (#5642)
  Fix internalUrl  Widget to Reflect Prop Changes via onChangeBlock (#6036)
  [types] Improve styleClassNameExtenders types, Icon component JSDoc t… (#6095)
  Fix link in pop-up RelationsMatrix.jsx (#6085)
  Fix deselecting lists. (#6080)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants