Only include the original image in the Image component if the image does not have all the scales present. Fixed and update tests.#7655
Conversation
…oes not have all the scales present. Fixed and update tests.
davisagli
left a comment
There was a problem hiding this comment.
The presence of the original image didn't force the browser to choose it. But we set width and height to the original size and in most cases didn't set sizes, so the browser didn't have any way to know it could use a smaller scale.
I think setting sizes appropriately would allow the browser's scale selection to work better even if we did always include the original image. I have a slight preference for leaving the original image there, because if we remove it then it takes extra work to use it in the use cases where it's really what you need (i.e. full-width on a large monitor)
| alt="" | ||
| loading="lazy" | ||
| responsive={true} | ||
| sizes="(max-width: 940px) 100vw, 940px" |
There was a problem hiding this comment.
Ideally we should use a smaller size when there are multiple columns.
I don't love hardcoding the default width, but I don't have a better suggestion. We can't use CSS properties because the whole point of sizes is to give the browser enough information to pick the right image before CSS has loaded.
Co-authored-by: Steve Piercy <web@stevepiercy.com>
|
@stevepiercy I accepted all the suggestions, but David is working in the backend side, and we might amend them a bit the next days. Thanks! |
|
@sneridagh @davisagli please ping me again for a review when it's done cooking. |
|
Can someone else fix the broken links? I'm swamped. https://github.com/plone/volto/actions/runs/19663404467/job/56314471303?pr=7655 |
|
@stevepiercy it seems medium added some antibot measures... we cannot do anything about it. How do we proceed with this use cases? |
|
@sneridagh for Medium, ignore the entire domain with a regex in For the others, fix the links that redirect. |
…olto into donotaddoriginalimagetothesrcset * 'donotaddoriginalimagetothesrcset' of github.com:plone/volto: Apply suggestions from code review
|
@davisagli once plone/plone.volto#205 is merged and released, we can go for this one. We might want to amend the docs with the released p.volto version. @stevepiercy I amended the docs to reflect the latest changes. |
davisagli
left a comment
There was a problem hiding this comment.
@sneridagh I took another look at this. I still think it would be nice to make the following improvements:
- Add a config setting for the widths, instead of hardcoding 940px. (Also ideally inject them from the config setting into the CSS properties to avoid duplication, but we can live without that.)
- Give the teaser block contextual information about how many columns there are, so it can use smaller sizes when there are multiple columns.
But, I would be okay with merging this and adding those improvements incrementally.
|
I would say that for the Our usecase was the following: The |
|
@erral This still includes the original image in the srcset unless it is enormous (width greater than the largest defined scale size, which is 4000 pixels in Plone 6.2) |
* main: (122 commits) Release 19.0.0-alpha.27 Release @plone/slate 19.0.0-alpha.12 Release @plone/razzle 1.0.0-alpha.1 We do not release @plone/client from here Handle create-user 500 responses where body contains 'message' instead of 'error' (#7946) Dropzone filewidget a11y (#7956) Update css-minimizer-webpack-plugin to v7 and keep razzleOptions.enableSourceMaps control (#7934) Refactor Password Reset: Convert Class-Based Component to Functional Component (#7697) fix: Fixed searchable types in Object Browser Widget, adding also sel… (#7942) Fix in-page drag-and-drop for the Listing block in Volto 18 and 19 (#7907) Improved docs of Plone REST API JavaScript Client (#5576) Fix Moderate Comments control panel visibility based on Discussion Support addon installation (#7878) fix: Add authorization error handling to control panels (#7807) Fix subordering order tab (#7937) (#7939) Fix converting value to html in HtmlSlateWidget (#7938) razzle.config.js: Fix defaultPlugins missing name property to enable addon SCSS plugin replacement (#7908) Update users controlpanel to work with new response format (#7895) Avoid storing blocksClipboard in localstorage until it has a value (#7925) Remove link to Jobfamilie MEDICE: site is no longer a Volto site (redirects to non-Volto destination) (#7929) Exclude `.storybook` from ESLint's default hidden directory ignore list via `.eslintignore` to allow linting of Storybook configuration files (#7898) ...
stevepiercy
left a comment
There was a problem hiding this comment.
What happened to the Upgrade Guide entry from 8fe53d2 ? It's also not on main in https://6.docs.plone.org/volto/upgrade-guide/index.html. Search for "humongous"
|
@stevepiercy Good catch, the upgrade guide was accidentally lost in a bad merge. I re-added it in ddb87a8 |
|
Merged! Thanks to all involved! |
…oes not have all the scales present. Fixed and update tests. (plone#7655) Co-authored-by: Steve Piercy <web@stevepiercy.com> Co-authored-by: David Glick <david@glicksoftware.com>
Follow up: #7486 (Seven PR):