-
-
Notifications
You must be signed in to change notification settings - Fork 182
Document images test view #1992
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
Conversation
stevepiercy
left a comment
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.
This is good stuff. My suggestions are mostly formatting and style.
Let's wait for a technical review, too.
Co-authored-by: Steve Piercy <web@stevepiercy.com>
stevepiercy
left a comment
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.
Looks like we had too many cooks and some suggestions were overlooked. Please take another look. If you find resolved suggestions that you didn't resolve, then please unresolve them until they are addressed. Thank you!
|
Do you think we should add images for 'Picture tags' and 'srcset attributes' as well? |
Co-authored-by: Steve Piercy <web@stevepiercy.com>
|
I think I got all of them now |
stevepiercy
left a comment
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.
This is looking good. A couple more changes to images. I'd also like the images to be 760px wide to fill in the width of the card, else it has a weird empty space to its right at https://plone6--1992.org.readthedocs.build/classic-ui/images.html#images-test-rendering
@Manas-Kenge I think that's a good idea. Please see the guidance I gave in my review about screenshot width. @petschki would you please do one more review? Here's the PR preview link: https://plone6--1992.org.readthedocs.build/classic-ui/images.html#images-test-rendering |
petschki
left a comment
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
stevepiercy
left a comment
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 align the alt tags and captions with the titles in the images. Then let's merge. Thank you!
|
I'm going to wait until plone/plone.api#591 is merged to make sure that the docs build completely. I enabled auto-merge. |
|
All green. Thank you! |
Issue number
@@images-testview #1921Description
Document @@images-test view
📚 Documentation preview 📚: https://plone6--1992.org.readthedocs.build/