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

Refactored Blocks/ToC/Edits.jsx to functional component #6167

Merged
merged 11 commits into from
Aug 10, 2024

Conversation

Prince0906
Copy link
Contributor

@Prince0906 Prince0906 commented Jul 13, 2024

Ref: #4460

About This PR

This PR refactored

packages/volto/src/components/manage/Blocks/ToC/Edit.jsx

from class based to functional component

Before:

WhatsApp.Video.2024-07-13.at.13.44.33.mp4

After

WhatsApp.Video.2024-07-13.at.13.43.58.mp4

This is my first time contrubuting to Volto .So,if I have done anything wrong in my PR, please give feedback and I will try to rectify that.

Copy link

netlify bot commented Jul 13, 2024

Deploy Preview for plone-components canceled.

Name Link
🔨 Latest commit 03fe0bb
🔍 Latest deploy log https://app.netlify.com/sites/plone-components/deploys/66b6e4648d180a000889396f

@Prince0906
Copy link
Contributor Author

@stevepiercy Can you review my pr, it's ready to preview.

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.

This needs review from a member of the @plone/volto-team.

I noticed several recent pull requests that provide similar refactors from newcomers. I'm curious whether this PR is part of a GSoC project or some other recruitment or discovery?

packages/volto/news/6167.feature Outdated Show resolved Hide resolved
Co-authored-by: Steve Piercy <[email protected]>
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.

LGTM.

@Prince0906
Copy link
Contributor Author

This needs review from a member of the @plone/volto-team.

I noticed several recent pull requests that provide similar refactors from newcomers. I'm curious whether this PR is part of a GSoC project or some other recruitment or discovery?

I am just exploring this things with my friends.

@stevepiercy
Copy link
Collaborator

@Prince0906 you do not need to repeatedly @ individual people. It is considered annoying and rude. I have already tagged the appropriate team.

@ichim-david
Copy link
Member

@Prince0906 Merging a pull request takes time and you need to be patient. There are some extra things to take into consideration when something is merged such as the need to mark something as breaking or not, or to think on what release version this change should be added.

The good news is that you have 2 approved reviews so this work will be merged however asking sneridan to review it
will not get your pull request merged quickly, instead you will get a notice about not repeatably messaging someone.

I know that you are anxious to have this merged but again that takes time. While waiting if you see another issue that you would know how to tackle feel free to do so and we can help then just like we did now.

@Prince0906
Copy link
Contributor Author

@Prince0906 Merging a pull request takes time and you need to be patient. There are some extra things to take into consideration when something is merged such as the need to mark something as breaking or not, or to think on what release version this change should be added.

The good news is that you have 2 approved reviews so this work will be merged however asking sneridan to review it will not get your pull request merged quickly, instead you will get a notice about not repeatably messaging someone.

I know that you are anxious to have this merged but again that takes time. While waiting if you see another issue that you would know how to tackle feel free to do so and we can help then just like we did now.

Sorry for the mistake, I will be patient and will avoid making this mistake again.

@stevepiercy
Copy link
Collaborator

I went through the original PLIP and found that this PR may duplicate work in #4961.

@Prince0906
Copy link
Contributor Author

I went through the original PLIP and found that this PR may duplicate work in #4961.

In that pr , Because of some other fixing all checks are not passing.

@stevepiercy
Copy link
Collaborator

@Prince0906 OK. Thanks for confirming. I've put this (and dozens of other) PRs on our next Volto Team Meeting agenda. @plone/volto-team needs to do better with its management of PRs.

@Prince0906
Copy link
Contributor Author

@stevepiercy I had updated the branch and this failed check is not related to my component.

@stevepiercy
Copy link
Collaborator

@Prince0906 thanks for the update and confirming your results against the CI failing check. I restarted the failing check as it has a flaky test in it (metadata).

@stevepiercy
Copy link
Collaborator

Flaky test passed. We need a final review before merging from @plone/volto-team to see if it is a breaking change, and compare against #4961.

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.

Thanks for your contribution!

I'll go ahead and merge this, and take a separate look at #4961, which also includes a fix that is unrelated to the refactoring.

@davisagli davisagli merged commit dab9fe5 into plone:main Aug 10, 2024
44 checks passed
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.

4 participants