-
-
Notifications
You must be signed in to change notification settings - Fork 703
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:Order blocks in sidebar doesn't respect layout restrictions: requ… #6489
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for plone-components canceled.
|
719a285
to
10fb29c
Compare
@plone/volto-team I have made the appropriate changes to fix the issue and It works as wanted please If you can review this PR. |
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.
Please see my note about the change log entry.
Otherwise, this looks really good, but it should get a thorough technical review by the @plone/volto-team. Thank you!
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.
News looks good, still needs a technical review.
@alihamza1221 We had the yearly Plone Conference Last week in Brasilia, BR, so most of our community members who are volunteers in this project were very busy with preparing, travelling and attending. It could therefore take a while before this is reviewed, but it will probably get on the next Volto Team meeting. As @sneridagh loves the new sidebar ordering he'll want improvements for that added asap ;-) (teasing him) |
@@ -91,6 +99,9 @@ export const Item = forwardRef( | |||
<button | |||
ref={ref} | |||
{...handleProps} | |||
style={{ |
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.
instead of having a visibility style it would make more sense to me to have the "disabled" property added and this way you also get a screen reader compatible information on why you can't interact with the button
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.
Looks good, but there are other places were we have used visibilty style will update it today
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.
@ichim-david I did added disabled property but removing visiblity style it's didn't seemed to respect the required functionality, If you would like to add more changes let me know
71de298
to
51985c8
Compare
fixed : Order blocks in sidebar doesn't respect layout restrictions: required and fixed position
I added the checks to display the delete block and drag block button according to values set like fixed and required.
After my change:
order.block.fixes.video.mp4
Closes #6481