- Sponsor
-
Notifications
You must be signed in to change notification settings - Fork 176
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
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! |
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.
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 .
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. |
- Avoid enumerating headings. - Use complete sentences and headings that align with the content.
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.
good!
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.
LGTM @petschki @Manas-Kenge @stevepiercy finally fine…
Background: I was waiting for over 10 years to have this finally in the docs. There is nothing done unless you do it. (Shame on me). In this case I at least wrote the ticket now. And @Manas-Kenge picked it up. When Ramon and Albert were working on Barceloneta they were surprised when I showed it to them in Sorrento. But then I left it to others to make it into the docs. |
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 just noticed that these images are too wide to display clearly in the portal. Please see https://6.docs.plone.org/contributing/documentation/myst-reference.html#width-of-media.
You can either crop out the left toolbar or collapse it.
I also found a bug and opened an issue in plone-sphinx-theme for it, but that's beyond the scope of this PR.
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.
Please resize the browser such that images are no wider than 760 pixels, as described in https://6.docs.plone.org/contributing/documentation/myst-reference.html#width-of-media
There is also for any type with image fields the |
https://classic.demo.plone.org/@@images-test 😢 @jensens got a valid link to view? |
in theory this should work, but its broken, hmm, smells like a bug |
And an old one at that plone/plone.namedfile#125. Until it's resolved, we shouldn't document it. |
@mauritsvanrees corrected me, and created a new issue plone/plone.namedfile#173. |
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.
You need to resize the browser window, then take the screenshot to fit the browser area that matters. Downsizing an image worsens its legibility. Was that not clear in the documentation?
https://6.docs.plone.org/contributing/documentation/myst-reference.html#width-of-media
Hey @stevepiercy, I did as you asked, but the image quality hasn't improved even after taking the screenshots. |
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 we go! Thank you! Merging when green.
I have released Plone 6.1.1 final, and Philip has updated the Classic demo site and this now works. |
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