-
-
Notifications
You must be signed in to change notification settings - Fork 726
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):App extras exceptions #5621
Conversation
✅ Deploy Preview for plone-components canceled.
|
✅ Deploy Preview for volto ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Also, if this is merged, can we backport it to version 16.x? |
This needs narrative documentation. See https://6.docs.plone.org/volto/recipes/appextras.html. I would like to see an exclude code example, modeled on the default code example. Additionally, we need to include a placeholder for this admonition, especially if it gets backported.
Side note, that page has some syntax and grammar issues that could be addressed at the same time. |
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.
The matching rule isn't correct, look at the test output, now every test returns null which means that your condition always renders true.
@ichim-david, you're right, the default value was incorrect. |
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.
@dobri1408 you need to add a test for this feature
@@ -9,8 +9,9 @@ beforeAll(() => { | |||
match: { | |||
path: '', | |||
}, | |||
exclude: '/blog', |
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.
@dobri1408 do not put your code that you want to test inside the beforeAll call which is called for each test.
Write a new test that passes the exclude and see how it behaves leaving the other tests as they were before you worked on this feature.
packages/volto/src/components/theme/AppExtras/AppExtras.test.jsx
Outdated
Show resolved
Hide resolved
@dobri1408 don't forget to also do this step that @stevepiercy mentioned that you need todo |
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 I would prefer a setting called ignore
, rather then exclude
.
out of curiosity, the react-router-dom |
Hi @sneridagh! This was my first thought, but unfortunately no, it doesn't support regex so we have to introduce this new property. |
@dobri1408 please perform the suggestions from @ichim-david and @tiberiuichim I think they are valuable. |
Co-authored-by: ichim-david <ichim.david@gmail.com>
…into app-extras-exceptions
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.
These suggestions might need more revision, and should be reviewed by someone who can make sure the rewording I suggested aligns with the code functionality. Thank you!
Co-authored-by: Steve Piercy <web@stevepiercy.com>
Co-authored-by: Steve Piercy <web@stevepiercy.com>
Co-authored-by: Steve Piercy <web@stevepiercy.com>
Co-authored-by: Steve Piercy <web@stevepiercy.com>
Co-authored-by: Steve Piercy <web@stevepiercy.com>
Co-authored-by: Steve Piercy <web@stevepiercy.com>
Co-authored-by: Steve Piercy <web@stevepiercy.com>
Co-authored-by: Steve Piercy <web@stevepiercy.com>
Co-authored-by: Steve Piercy <web@stevepiercy.com>
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.
One more change, after checking the preview on Netlify.
Co-authored-by: Steve Piercy <web@stevepiercy.com>
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.
Thank you!
@sneridagh this is ready to merge for some time and everyone gave their approval, can we merge this or do you want to merge something else before? |
…bars-et-al * main: (56 commits) Exclude chromewebstore from linkcheck (#5761) (feat):App extras exceptions (#5621) Add VOLTOCONFIG Env Var (#5752) PoC - Vite and Tanstack Router, Query and `@plone/client` in SSR mode. (#5750) Update links to Redux and React developer extensions for Chrome (#5757) Overhaul environment variables documentation (#5736) Mention what version the 'links and references' view was added (#5756) Add wait commands to flaky block-listing tests (#5753) Fix logging in test-acceptance-server commands (#5748) Replaced outdated diff with a link to the current `package.json` on t… (#5728) Listing Block render of initial results in SSR (#5689) Fix @plone/volto-slate path in moduleNameMapper (#5743) Added global form state. (#5721) Release generate-volto 9.0.0-alpha.5 Fix tests in projects that involves TS files (#5738) Reorganize README, merging content into authoritative locations (#5511) Release 18.0.0-alpha.10 Release generate-volto 9.0.0-alpha.4 Release @plone/registry 1.2.2 Enhance release in @plone/registry ...
Co-authored-by: ichim-david <ichim.david@gmail.com> Co-authored-by: Steve Piercy <web@stevepiercy.com>
In situations where we do not want the appExtras rule to apply universally to all routes matched by a certain rule, the addition of an exclude property could be beneficial. This property would provide a way to specify exceptions to the rule. For instance:
{ match: '*', exclude: '/**/diff', component: RemoveSchema, }
To address this scenario, where the goal is to apply a rule to all pages except for the "history diff" page, incorporating an exclude property into the rule configuration is an effective solution.