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

Add setting unwantedControlPanelsFields and use it in the function filterControlPanelsSchema #4819

Merged
merged 4 commits into from
Aug 24, 2024

Conversation

wesleybl
Copy link
Member

filterControlPanelsSchema has been migrated to Controlpanel.jsx and uses the unwantedControlPanelsFields setting. So, if any portal wants to add fields in the Control Panel, it will be necessary to customize only the unwantedControlPanelsFields configuration, instead of customizing the entire filterControlPanelsSchema function.

This is an improvement for #3904

@netlify
Copy link

netlify bot commented May 25, 2023

Deploy Preview for volto ready!

Name Link
🔨 Latest commit a49236f
🔍 Latest deploy log https://app.netlify.com/sites/volto/deploys/66201975e760c10008518bbf
😎 Deploy Preview https://deploy-preview-4819--volto.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@cypress
Copy link

cypress bot commented May 25, 2023

Passing run #5325 ↗︎

0 493 20 0 Flakiness 0

Details:

Replaces filterControlPanelsSchema setting by
Project: Volto Commit: 81a6aaaddb
Status: Passed Duration: 14:56 💡
Started: May 25, 2023 4:21 PM Ended: May 25, 2023 4:36 PM

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

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.

Thanks for updating docs! 🤗

I reviewed docs only. A maintainer should review the source code.

docs/source/configuration/settings-reference.md Outdated Show resolved Hide resolved
@@ -0,0 +1 @@
Replaces ``filterControlPanelsSchema`` setting by ``unwantedControlPanelsFields`. @wesleybl
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
Replaces ``filterControlPanelsSchema`` setting by ``unwantedControlPanelsFields`. @wesleybl
Renames `filterControlPanelsSchema` setting to `unwantedControlPanelsFields`. @wesleybl

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.

Removing support for a custom filterControlPanelsSchema function is a breaking change that could only happen in a new major version of volto. I'd suggest keeping it, but change the default implementation to use the new unwantedControlPanelFields setting. Then it can be backported for volto 16.

@wesleybl
Copy link
Member Author

Removing support for a custom filterControlPanelsSchema function is a breaking change that could only happen in a new major version of volto.

@davisagli but isn't the master branch for Volto 17, which is a breaking release? Can't we keep filterControlPanelsSchema only on branch 16? If we don't remove it now, when will we remove it? How will we remember to remove it in the future?

@davisagli
Copy link
Member

@wesleybl Yeah, we can make this change for 17. In general my bias is to keep backwards compatibility if it's easy. But that does inflate the number of options over time, and in this particular case I doubt the filterControlPanelsSchema setting is being used much. Any preference, @sneridagh?

@sneridagh
Copy link
Member

@wesleybl I'm with @davisagli in this one... agreed that it cleans up the implementation, but that is hardly a reason for introducing a breaking if there is not delivering a clear direct advantage over the previous one.

@wesleybl wesleybl force-pushed the controlpanel_filter branch from 81a6aaa to 26c4c02 Compare April 16, 2024 19:44
Copy link

netlify bot commented Apr 16, 2024

Deploy Preview for plone-components canceled.

Name Link
🔨 Latest commit 35894b9
🔍 Latest deploy log https://app.netlify.com/sites/plone-components/deploys/66c3b5d694007700086bfd94

@wesleybl
Copy link
Member Author

@davisagli @sneridagh @stevepiercy I kept the filterControlPanelsSchema function and used unwantedControlPanelsFields in it.

Can you take a look please?

@wesleybl wesleybl force-pushed the controlpanel_filter branch from 04f2756 to a3d42a0 Compare April 16, 2024 20:41
@wesleybl wesleybl changed the title Replaces filterControlPanelsSchema setting by unwantedControlPanelsFields Add setting unwantedControlPanelsFields and use it in the function filterControlPanelsSchema Apr 16, 2024
docs/source/configuration/settings-reference.md Outdated Show resolved Hide resolved
docs/source/configuration/settings-reference.md Outdated Show resolved Hide resolved
packages/volto/news/4819.feature Outdated Show resolved Hide resolved
@wesleybl wesleybl requested a review from stevepiercy April 17, 2024 13:09
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.

One minor grammar correction to the narrative docs made me notice that there might be a grammatical mistake in the naming of the schema factory. Why do use filterControlPanelsSchema and unwantedControlPanelsFields instead of filterControlPanelSchema and unwantedControlPanelFields? Obviously it is not worth refactoring the deprecated filterControlPanelsSchema, but it might be worth refactoring the new factory to unwantedControlPanelFields. I understand that the endpoint is @controlpanels. Thoughts?

docs/source/configuration/settings-reference.md Outdated Show resolved Hide resolved
@wesleybl
Copy link
Member Author

@wesleybl wesleybl requested a review from stevepiercy April 17, 2024 18:40
@stevepiercy
Copy link
Collaborator

@wesleybl let's leave it as is without a refactor. Plural makes sense. I made a minor change to my suggestion, and if you accept, then I can approve and move this forward. Thank you!

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.

Thank you!

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.

This looks good to me now.

wesleybl and others added 4 commits August 19, 2024 18:10
unwantedControlPanelsFields

filterControlPanelsSchema has been migrated to Controlpanel.jsx and uses
the unwantedControlPanelsFields setting. So, if any portal wants to add
fields in the Control Panel, it will be necessary to customize only the
unwantedControlPanelsFields configuration, instead of customizing the
entire filterControlPanelsSchema function.
Co-authored-by: Steve Piercy <[email protected]>
@wesleybl wesleybl force-pushed the controlpanel_filter branch from a49236f to 35894b9 Compare August 19, 2024 21:15
@wesleybl
Copy link
Member Author

@davisagli @ichim-david I updated the code and resolved the conflicts. Can we merge it?

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.

Looks good to me.

@davisagli davisagli merged commit a79ebb4 into main Aug 24, 2024
71 checks passed
@davisagli davisagli deleted the controlpanel_filter branch August 24, 2024 04:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants