-
-
Notifications
You must be signed in to change notification settings - Fork 172
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 /@@test-rendering views to docs #1903
base: 6.0
Are you sure you want to change the base?
Conversation
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'll do a second review, but for now...
-
Please follow https://6.docs.plone.org/contributing/documentation/authors.html#plone-documentation-styleguide, specifically:
Headings should be "Sentence cased", not "Title Cased".
-
Don't inject content between a target
(classic-ui-template-slots-label)=
and its heading. Keep them close together. -
The correct proper name is "Classic UI" with a space.
-
Use 4 spaces indentation in all list items, else they don't render correctly.
-
This content breaks flow of the original document. You must consider where it belongs, and not throw it in wherever.
-
A picture is worth a thousand words. Screenshots would be nice. See guidance in https://6.docs.plone.org/contributing/documentation/myst-reference.html#images-and-figures
@plone/classicui-team could I get a review from y'all on content accuracy, and where this documentation should reside? It's in the wrong place at the moment. Thank you! |
docs/classic-ui/views.md
Outdated
- Grid layout examples | ||
- Form control variants | ||
- Navigation components | ||
- Interactive element states |
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'd also mention the colormode switcher here
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.
@petschki I cannot find any colormode text in the templates. Any hints?
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 page https://6.docs.plone.org/classic-ui/theming/color-mode.html is listed by google, but not present (404).
The Light/dark/auto Toggle Button is mentioned in https://6.docs.plone.org/classic-ui/theming/color-modes.html#toggle-button
I guess you mean "mentioning" means to point out, that the appearance of the examples differs depending on your day/night dark/light theme setting.
This is a halfbaked admonition. @stevepiercy can wordsmith a better one.
```{important}
Please take into consideration how to make sure checking the examples includes both the dark and light setting when testing the UI.
```
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'd suggest to put this somewhere into classic-ui/theming/create-add-on.md
as subsection Styles Test Rendering
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.
@Manas-Kenge Thx for this initial iteration. Maybe we need another round.
docs/classic-ui/views.md
Outdated
- Grid layout examples | ||
- Form control variants | ||
- Navigation components | ||
- Interactive element states |
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.
@petschki I cannot find any colormode text in the templates. Any hints?
docs/classic-ui/views.md
Outdated
- Grid layout examples | ||
- Form control variants | ||
- Navigation components | ||
- Interactive element states |
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 page https://6.docs.plone.org/classic-ui/theming/color-mode.html is listed by google, but not present (404).
The Light/dark/auto Toggle Button is mentioned in https://6.docs.plone.org/classic-ui/theming/color-modes.html#toggle-button
I guess you mean "mentioning" means to point out, that the appearance of the examples differs depending on your day/night dark/light theme setting.
This is a halfbaked admonition. @stevepiercy can wordsmith a better one.
```{important}
Please take into consideration how to make sure checking the examples includes both the dark and light setting when testing the UI.
```
It is indeed important information for theming Classic UI. |
I am lost how to review latest changes due to confusion if a test build exists or how to checkout the latest state of the pr locally. If I am the origin of that mess, get me out of the ditch… ;-) |
@acsr I'm not sure exactly what you want to review, but to see the rendered HTML, visit the pull request preview at https://plone6--1903.org.readthedocs.build/classic-ui/theming/create-add-on.html#styles-test-rendering. If a repo supports pull request previews, then Read the Docs will insert a link in the PR description, which I then usually modify to point to the affected files for convenience. To checkout this branch, you first need to add a git remote |
I did exactly this (the link at the top of the PR) but it did not work for me. (I had this several times, that these links seem broken. I thought it was due to they were outdated because of changes after a review.) But it is possible that Github ditches them after some 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.
@Manas-Kenge THX! Everything else looks good to me… except the orphaned Colormode switcher
hint, that needs more love. I suggest taking it out and continue in a seperate iteration issued by @petschki .
- Form control variants | ||
- Navigation components | ||
- Interactive element states | ||
- Colormode switcher |
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.
As I wrote before in my last review:
There is no Colormode switcher
mentioned in the current Plone6.1 cheatsheet at all.
Not even the word color
is present to search on the page. Also no manual dark/light colormode switcher is visible by default in Classic UI, in the personal prefs or as action that could be activated. This needs more explanation or to be removed.
The Colormode switcher
item was requested by @petschki very vague in this review comment:
I'd also mention the colormode switcher here
@petschki -> Can you please clarify / refine that?
The Light/dark/auto Toggle Button is mentioned in https://6.docs.plone.org/classic-ui/theming/color-modes.html#toggle-button
see also from my comment: #1903 (comment) :
I guess you mean "mentioning" means to point out, that the appearance of the examples differs depending on your day/night dark/light theme setting.
This is a halfbaked admonition. @stevepiercy can wordsmith a better one.
Please take into consideration how to make sure checking the examples includes both the dark and light setting when testing the UI.
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.
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.
@Manas-Kenge Wait one day and then take it out. I push him ;-)
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.
It can be added in a next iteration again. I have no clue what to add except my suggested admonition.
RTD inserts the link immediately when it starts the build, not when the build completes. So if you visit the link within 5 minutes of the PR creation, then you might get a 404. Otherwise, it should always work. It works for me now. |
Issue number
Description
This is a WIP, need an initial review to keep working. As per #1871 (comment) a mention of both test-rendering and Storyboard docs in the migration docs under Migrating from Plone Classic UI to Volto is remaining.
📚 Documentation preview 📚: https://plone6--1903.org.readthedocs.build/classic-ui/theming/create-add-on.html#styles-test-rendering