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

Add /@@test-rendering views to docs #1903

Merged
merged 17 commits into from
Mar 22, 2025

Conversation

Manas-Kenge
Copy link
Contributor

@Manas-Kenge Manas-Kenge commented Mar 13, 2025

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

Copy link
Contributor

@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'll do a second review, but for now...

@stevepiercy stevepiercy requested a review from acsr March 13, 2025 09:07
@stevepiercy
Copy link
Contributor

@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!

@acsr acsr changed the title Add /@@test-rendering views to docs Add /@@test_rendering views to docs Mar 13, 2025
@acsr acsr changed the title Add /@@test_rendering views to docs Add /@@test-rendering views to docs Mar 13, 2025
Copy link
Member

@petschki petschki left a 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

Copy link
Contributor

@acsr acsr left a 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.

@stevepiercy stevepiercy requested review from acsr and petschki March 17, 2025 00:15
@jensens
Copy link
Member

jensens commented Mar 18, 2025

It is indeed important information for theming Classic UI.

@acsr
Copy link
Contributor

acsr commented Mar 18, 2025

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… ;-)

@stevepiercy
Copy link
Contributor

@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 https://github.com/Manas-Kenge/documentation.git, then git fetch --all, and finally checkout the branch. You can then view git commit history.

@acsr
Copy link
Contributor

acsr commented Mar 18, 2025

visit the pull request preview at https://plone6--1903.org.readthedocs.build/classic-ui/theming/create-add-on.html#styles-test-rendering.

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.

Copy link
Contributor

@acsr acsr left a 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 .

@stevepiercy
Copy link
Contributor

visit the pull request preview at https://plone6--1903.org.readthedocs.build/classic-ui/theming/create-add-on.html#styles-test-rendering.

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.

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.
@stevepiercy stevepiercy requested a review from acsr March 19, 2025 08:21
@stevepiercy
Copy link
Contributor

I'm happy with this content after making a few commits. @acsr or @petschki one more pass, please? Thank you!

petschki
petschki previously approved these changes Mar 19, 2025
Copy link
Member

@petschki petschki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good!

acsr
acsr previously approved these changes Mar 19, 2025
Copy link
Contributor

@acsr acsr left a 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…

@acsr
Copy link
Contributor

acsr commented Mar 19, 2025

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.

Copy link
Contributor

@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 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.

@Manas-Kenge Manas-Kenge dismissed stale reviews from acsr and petschki via 79b6abe March 20, 2025 04:15
Copy link
Contributor

@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.

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

@jensens
Copy link
Member

jensens commented Mar 20, 2025

There is also for any type with image fields the @@images-test view from plone.namedfile.

@stevepiercy
Copy link
Contributor

There is also for any type with image fields the @@images-test view from plone.namedfile.

https://classic.demo.plone.org/@@images-test 😢

@jensens got a valid link to view?

@jensens
Copy link
Member

jensens commented Mar 20, 2025

in theory this should work, but its broken, hmm, smells like a bug
https://classic.demo.plone.org/en/demo/an-image.jpg/@@images-test

@stevepiercy
Copy link
Contributor

in theory this should work, but its broken, hmm, smells like a bug https://classic.demo.plone.org/en/demo/an-image.jpg/@@images-test

And an old one at that plone/plone.namedfile#125.

Until it's resolved, we shouldn't document it.

@stevepiercy
Copy link
Contributor

@mauritsvanrees corrected me, and created a new issue plone/plone.namedfile#173.

Copy link
Contributor

@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.

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

@Manas-Kenge
Copy link
Contributor Author

Hey @stevepiercy, I did as you asked, but the image quality hasn't improved even after taking the screenshots.

@stevepiercy
Copy link
Contributor

You did not resize your browser width. In this screenshot, you can see that I resized my browser width such that the area I want to capture has a width of 760 pixels.

Screenshot 2025-03-21 at 12 14 14 AM

I would then get a screenshot of only that area I needed, without all the navigation.

Screenshot 2025-03-21 at 12 15 33 AM

Copy link
Contributor

@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.

There we go! Thank you! Merging when green.

@stevepiercy stevepiercy merged commit ffda6cf into plone:6.0 Mar 22, 2025
2 checks passed
@mauritsvanrees
Copy link
Member

in theory this should work, but its broken, hmm, smells like a bug https://classic.demo.plone.org/en/demo/an-image.jpg/@@images-test

I have released Plone 6.1.1 final, and Philip has updated the Classic demo site and this now works.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Add a mention of the /@@test-rendering views to the developer documentation for Classic UI
6 participants