-
-
Notifications
You must be signed in to change notification settings - Fork 725
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
Conversation
✅ Deploy Preview for plone-components canceled.
|
There was a problem hiding this 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.
Co-authored-by: Steve Piercy <[email protected]>
Co-authored-by: Steve Piercy <[email protected]>
we need it. it was with / already before my changes. Co-authored-by: Mauro Amico <[email protected]>
Co-authored-by: Steve Piercy <[email protected]>
There was a problem hiding this 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.
@mamico @giuliaghisini This doesn't look ready for testing yet due to the unit test failure |
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! |
@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. |
updated how-to file |
There was a problem hiding this 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.
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]>
There was a problem hiding this 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.
There was a problem hiding this 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.
@sneridagh could we merge this in volto 18? |
@plone/volto-team I am +1 on having such a feature however I am not completely sold on the new setting name. If we were to have good typescript integration I think that having the same base would make it easier to find every option used. |
I tend to follow the same naming convention suggested by @ichim-david in my projects: If a renaming refactor is done, I prefer the shorter proposed version |
@stevepiercy @ichim-david @sneridagh i think it's ready now |
There was a problem hiding this 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.
Co-authored-by: Steve Piercy <[email protected]>
Co-authored-by: Steve Piercy <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's merge!
I restarted the lincheck build as it had a temporary outage for a link. |
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:
defining this public routes in config we can avoid this and everithing works well :-D