-
-
Notifications
You must be signed in to change notification settings - Fork 739
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
Adding new text block via dropdown when current block is empty #6580
base: main
Are you sure you want to change the base?
Adding new text block via dropdown when current block is empty #6580
Conversation
✅ Deploy Preview for plone-components canceled.
|
@stevepiercy @sneridagh provide review on this. |
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 modified the change log so that it says both what was broken and fixed. Otherwise LGTM. Thanks for your work!
packages/volto/news/6574.feature
Outdated
@@ -0,0 +1 @@ | |||
Implemented functionality to add a new text block via the dropdown menu, even when the current text block is empty. @Abhishek-17h |
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.
Implemented functionality to add a new text block via the dropdown menu, even when the current text block is empty. @Abhishek-17h | |
You can now add a text block by clicking the circled `+` icon ("Add" block button) and selecting "Text" from the pop-up menu. Previously no text block would be inserted. @Abhishek-17h |
This is nice work. Let's get the @plone/volto-team and @plone/volto-accessibility teams to give a technical review before merging. |
onMutateBlock(block, { ...data, ...value }); | ||
setAddNewBlockOpened(false); | ||
} else { | ||
setAddNewBlockOpened(false); |
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 move setAddNewBlockOpened(false);
outside of the conditional, since it's done in both branches and the order doesn't matter.
82efe76
to
4629722
Compare
4629722
to
6b0dcb7
Compare
@davisagli @stevepiercy Please have a look now. |
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. Thanks for the review, @davisagli!
9ab1c4a
to
1349251
Compare
hey @davisagli, if everything is good ,can please u merge this PR. |
1349251
to
c10bc63
Compare
@Abhishek-17h Just out of curiosity, could you please tell me why do you have such rush in review and merging? Which are your motivations? |
@sneridagh It's not really about rushing ,my main intention is to ensure that the work I’ve done is on the right track. If it’s good enough, it can move forward. I’m still learning about how things are managed here, like whether PRs are discussed in meetings or asynchronously, so any guidance would be helpful. I apologise if I have done something wrong. |
The video showcases the behavior after the feature for clarity:
Screencast from 2025-01-12 12-21-31.webm
Closes #6574