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

feat: publicUi routes to not have cms-ui in public nonContentRoutes #6173

Merged
merged 26 commits into from
Sep 24, 2024

Conversation

giuliaghisini
Copy link
Contributor

@giuliaghisini giuliaghisini commented Jul 19, 2024

if we have nonContentRoutes that aren't cms-ui,
for example '/search'
now are considered as cmsUi.
This adds unnecessary things to the page like:

  • cms-ui class to the body tag

defining this public routes in config we can avoid this and everithing works well :-D

Copy link

netlify bot commented Jul 19, 2024

Deploy Preview for plone-components canceled.

Name Link
🔨 Latest commit 9f6e1e9
🔍 Latest deploy log https://app.netlify.com/sites/plone-components/deploys/66ed5ea3431a830008d6a44b

packages/volto/src/config/PublicUiRoutes.jsx Outdated Show resolved Hide resolved
packages/volto/src/config/index.js Outdated Show resolved Hide resolved
packages/volto/src/config/PublicUiRoutes.jsx Outdated Show resolved Hide resolved
packages/volto/src/helpers/Url/Url.js Outdated Show resolved Hide resolved
packages/volto/news/6173.feature Outdated Show resolved Hide resolved
packages/volto/src/config/PublicNonContentRoutes.jsx Outdated Show resolved Hide resolved
packages/volto/src/config/PublicUiRoutes.jsx Outdated Show resolved Hide resolved
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.

Clean up news and comments, and a couple of questions.

packages/volto/src/helpers/Url/Url.js Outdated Show resolved Hide resolved
packages/volto/src/config/PublicNonContentRoutes.jsx Outdated Show resolved Hide resolved
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.

@plone/volto-team this PR needs a technical review.

@ichim-david
Copy link
Sponsor Member

@mamico @giuliaghisini This doesn't look ready for testing yet due to the unit test failure
unitest-failures

@stevepiercy
Copy link
Collaborator

The failing CI check for readme-link-check is due to https://www.cmscom.jp not responding. @terapyon would you please look into this?

Otherwise it is all green, but could use one more round of reviews. @giuliaghisini would you please add your review? Thank you!

@ichim-david
Copy link
Sponsor Member

ichim-david commented Aug 17, 2024

@mamico you've turned a bugfix into a breaking change. If you modify where the config is at it will need a breaking notice and an upgrade mention. I would wait for this action though to hear what @davisagli thinks about your proposal to move the config setting.
EDIT:
Was too quick to react, I've added a reply here #6173 (comment)

@giuliaghisini
Copy link
Contributor Author

For docs, see https://6.docs.plone.org/volto/contributing/documentation.html.
@ichim-david as far as what to document and where, I don't see anything for nonContentRoutes in Routing (so sad), The configuration registry, or Settings reference guide. Would one of these be appropriate locations?

It would be good to get the setting documented and in the routing docs to link to the routing based settings

updated how-to file

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 expanded on @giuliaghisini's documentation. This should be reviewed by another contributor.

docs/source/configuration/how-to.md Outdated Show resolved Hide resolved
docs/source/configuration/how-to.md Outdated Show resolved Hide resolved
docs/source/configuration/how-to.md Outdated Show resolved Hide resolved
docs/source/configuration/how-to.md Outdated Show resolved Hide resolved
docs/source/configuration/how-to.md Show resolved Hide resolved
mamico and others added 5 commits August 22, 2024 19:56
Co-authored-by: Steve Piercy <[email protected]>
Co-authored-by: Steve Piercy <[email protected]>
Co-authored-by: Steve Piercy <[email protected]>
Co-authored-by: Steve Piercy <[email protected]>
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.

Thanks for checking the docs @mamico.

I think we should do one more technical review, as @mamico changes made after @pnicolli. Maybe @davisagli can fill in, or @pnicolli with a second review? Other folks are on vacation for another week or two.

Copy link
Sponsor 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.

I think this is okay for Volto 18. It's arguably a breaking change to backport it, since some existing sites may have styling that depends on the cms-ui class being there for these views.

@giuliaghisini
Copy link
Contributor Author

@sneridagh could we merge this in volto 18?

@ichim-david
Copy link
Sponsor Member

@plone/volto-team I am +1 on having such a feature however I am not completely sold on the new setting name.
If the new option would share the same base it would be easier to find the extension.
I'm thinking of naming such as:
nonContentRoutesPublic
nonContentRoutesPublicUI
nonContentRoutesOfPublicUI

If we were to have good typescript integration I think that having the same base would make it easier to find every option used.
Maybe someone has an even better naming example or the majority decides that publicNonContentRoutes is good enough, I am not strongly against the current suggestion but again I am not 100% sold either :).

@stevepiercy
Copy link
Collaborator

I tend to follow the same naming convention suggested by @ichim-david in my projects: BaseNameOptionalModifier or base-name-optional-modifier. We did something similar for almost all of the make commands.

If a renaming refactor is done, I prefer the shorter proposed version nonContentRoutesPublic, as these are routes. I don't think the UI segment helps, as it is an implementation detail better described in code comments.

@giuliaghisini
Copy link
Contributor Author

@stevepiercy @ichim-david @sneridagh
renamed var into nonContentRoutesPublic

i think it's ready 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.

Thanks for the refactor! This LGTM, with a few minor comment suggestions. I could not understand two of them, so I made a best guess.

packages/volto/src/config/NonContentRoutesPublic.jsx Outdated Show resolved Hide resolved
packages/volto/src/config/NonContentRoutesPublic.jsx Outdated Show resolved Hide resolved
docs/source/configuration/how-to.md Outdated Show resolved Hide resolved
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.

Let's merge!

@stevepiercy
Copy link
Collaborator

I restarted the lincheck build as it had a temporary outage for a link.

@pnicolli pnicolli merged commit 2dcf684 into main Sep 24, 2024
71 checks passed
@pnicolli pnicolli deleted the publicui-routes branch September 24, 2024 09:40
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.

6 participants