-
-
Notifications
You must be signed in to change notification settings - Fork 813
Refactored ButtonsWidget and AlignWidget. New blockWidth, blockAlignment and size widgets based on ButtonsWidget.
#7555
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
base: main
Are you sure you want to change the base?
Conversation
ButtonsWidget and AlignWidget. New blockWidth, blockAlignment and size widgets based on ButtonsWidget.
| */ | ||
| // import '../theme/themes/pastanaga-cms-ui/extras/cms-ui.semantic.less'; | ||
| import 'semantic-ui-less/semantic.less'; | ||
| import '@plone/components/dist/basic.css'; |
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.
@pnicolli we have to talk about this one.
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.
Yeah I noticed this one and yes we need to, this is a new problem to tackle.
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.
Let's also check the rendered and updated docs at:
- https://volto--7555.org.readthedocs.build/7555/reference/index.html
- https://volto--7555.org.readthedocs.build/7555/reference/widgets.html
- https://volto--7555.org.readthedocs.build/7555/upgrade-guide/index.html#alignwidget-and-buttonswidget-are-now-semantic-ui-free
- https://volto--7555.org.readthedocs.build/7555/upgrade-guide/index.html#size-blockwidth-and-blockalignment-widgets-added
| React.useEffect(() => { | ||
| if (!value && defaultValue) { | ||
| const nextValue = | ||
| typeof defaultValue === 'string' | ||
| ? normalizedActions.find(({ name }) => name === defaultValue) | ||
| ?.value ?? defaultValue | ||
| : defaultValue; | ||
|
|
||
| onChange(id, nextValue); | ||
| } | ||
| }, [defaultValue, id, normalizedActions, onChange, value]); |
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.
What is this for?
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.
It was necessary for managing the default value... This predates the "fixed" defaults in the blocks settings, so I haven't tried if that's necessary afterwards. Also, could be that RAC takes care for you, so you don't have to now. I will take a look.
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.
I removed it, and refactored to use RadioGroup defaultValue instead.
| [actions], | ||
| ); | ||
|
|
||
| const selectedActionName = React.useMemo( |
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.
This memoization feels unnecessary to me, do you have a situation where it's needed?
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.
Isnt't what the compiler is doing? The machine suggested that in case the options are big (for sure it would never be...)
| [normalizedActions, value], | ||
| ); | ||
|
|
||
| const handleChange = React.useCallback( |
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.
This one as well, not sure it's really needed
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.
Same here.
Co-authored-by: Steve Piercy <[email protected]>
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.
Docs look purty now.
This is the first time we will use
@plone/componentsin Volto core.We decided that in Volto 19, we will allow the usage of
@plone/components.