plone / volto

React-based frontend for the Plone Content Management System
https://demo.plone.org/
MIT License
446 stars 606 forks source link

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

Closed Prince0906 closed 4 weeks ago

Prince0906 commented 1 month ago

Ref: #4460

About This PR

This PR refactored

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

from class based to functional component

Before:

https://github.com/user-attachments/assets/84673f37-1e76-4382-a247-02ac82645ea4

After

https://github.com/user-attachments/assets/b007d68f-960b-4105-b9b1-682136457548

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.

netlify[bot] commented 1 month ago

Deploy Preview for plone-components canceled.

Name Link
Latest commit 03fe0bb0b82725c29d090c012963a863c00cdcf7
Latest deploy log https://app.netlify.com/sites/plone-components/deploys/66b6e4648d180a000889396f
Prince0906 commented 1 month ago

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

Prince0906 commented 1 month ago

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 commented 1 month ago

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

ichim-david commented 1 month ago

@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 commented 1 month ago

@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 commented 1 month ago

I went through the original PLIP and found that this PR may duplicate work in https://github.com/plone/volto/pull/4961.

Prince0906 commented 1 month ago

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 commented 1 month ago

@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 commented 1 month ago

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

stevepiercy commented 1 month ago

@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 commented 1 month ago

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.