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

Adding new text block via dropdown when current block is empty #6580

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Abhishek-17h
Copy link
Contributor


The video showcases the behavior after the feature for clarity:
Screencast from 2025-01-12 12-21-31.webm

Closes #6574

Copy link

netlify bot commented Jan 12, 2025

Deploy Preview for plone-components canceled.

Name Link
🔨 Latest commit c10bc63
🔍 Latest deploy log https://app.netlify.com/sites/plone-components/deploys/678f3e26ab82d600080dd59e

@Abhishek-17h
Copy link
Contributor Author

@stevepiercy @sneridagh provide review on this.

Copy link
Collaborator

@stevepiercy stevepiercy left a 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!

@@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
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

@stevepiercy
Copy link
Collaborator

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);
Copy link
Member

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.

@Abhishek-17h Abhishek-17h force-pushed the feature/enable-dropdown-for-empty-text-block branch from 82efe76 to 4629722 Compare January 12, 2025 07:46
@Abhishek-17h Abhishek-17h force-pushed the feature/enable-dropdown-for-empty-text-block branch from 4629722 to 6b0dcb7 Compare January 12, 2025 07:53
@Abhishek-17h
Copy link
Contributor Author

@davisagli @stevepiercy Please have a look now.

Copy link
Collaborator

@stevepiercy stevepiercy left a 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!

@Abhishek-17h Abhishek-17h force-pushed the feature/enable-dropdown-for-empty-text-block branch 2 times, most recently from 9ab1c4a to 1349251 Compare January 18, 2025 09:31
@Abhishek-17h
Copy link
Contributor Author

hey @davisagli, if everything is good ,can please u merge this PR.

@Abhishek-17h Abhishek-17h force-pushed the feature/enable-dropdown-for-empty-text-block branch from 1349251 to c10bc63 Compare January 21, 2025 06:26
@sneridagh
Copy link
Member

@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?

@Abhishek-17h
Copy link
Contributor Author

Abhishek-17h commented Jan 21, 2025

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Addition of text block
4 participants