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 image scale property to Teaser Block schema (#4653) #4706

Closed

Conversation

cihanandac
Copy link

This commit adds a new property called 'Image scale' to the schema of the Teaser Block in Volto. This property allows for the size of the image in the block to be adjusted based on its alignment. The default value for it is 'teaser' as it was originally set to this value. If the image align is not center, the scale will be set by this property. If the image align is center, the scale will be set to 'great'.

Closes #4653

@netlify
Copy link

netlify bot commented Apr 17, 2023

Deploy Preview for volto canceled.

Name Link
🔨 Latest commit dd673eb
🔍 Latest deploy log https://app.netlify.com/sites/volto/deploys/6441bc0edd78040008ce1dae

@erral erral requested review from libargutxi and ionlizarazu April 18, 2023 07:46
@@ -1,14 +1,14 @@
import { isInternalURL } from '@plone/volto/helpers';
import config from '@plone/volto/registry';

export function getTeaserImageURL({ href, image, align }) {
export function getTeaserImageURL({ href, image, align, scale }) {
Copy link
Member

@erral erral Apr 18, 2023

Choose a reason for hiding this comment

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

To keep backwards compatibility in the function, I think that the scale parameter should get the default value.

export function getTeaserImageURL({ href, image, align, scale = 'teaser' }) {

['teaser', 'Teaser'],
['large', 'Large'],
['great', 'Great'],
],
Copy link
Member

Choose a reason for hiding this comment

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

The scale names should be passed through intl to get them translated.

For future reuse of the scale list, perhaps this list should be configurable in the block configuration, this way a developer can change the list of the available scales without overriding the component.

Moreover, right now we do not have an API endpoint to expose the available image scales, but in the future I think that the list of available scales should come from the API endpoint call.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the review. I will keep this in mind for the future contributions.

@davisagli
Copy link
Member

I don't think this is the right approach. volto should serve an img with a srcset so that the browser can choose the best scale to download based on the actual image size (which can vary in a responsive design). That also keeps things simpler for the editor since they don't need to remember to pick the right scale size to match the alignment.

@erral
Copy link
Member

erral commented Apr 19, 2023

I don't think this is the right approach. volto should serve an img with a srcset so that the browser can choose the best scale to download based on the actual image size (which can vary in a responsive design). That also keeps things simpler for the editor since they don't need to remember to pick the right scale size to match the alignment.

That's a breaking change, right?

@nileshgulia1
Copy link
Member

There are some efforts here by redturtle: #2103

@erral
Copy link
Member

erral commented Apr 20, 2023

There are some efforts here by redturtle: #2103

Thanks for the pointer @nileshgulia1

I think that until the srcset effort is concluded and a way to handle it is decided, we should keep going with this PR.

@erral
Copy link
Member

erral commented Apr 20, 2023

I don't think this is the right approach. volto should serve an img with a srcset so that the browser can choose the best scale to download based on the actual image size (which can vary in a responsive design). That also keeps things simpler for the editor since they don't need to remember to pick the right scale size to match the alignment.

That's a breaking change, right?

Moreover, the Teaser block image component can be overridden using the component registry: see https://6.docs.plone.org/volto/blocks/core/teaser.html#image-component

@erral
Copy link
Member

erral commented Apr 20, 2023

I would also add some documentation about the Block, explaining how to modify the available image scale list, and also explaining the imageScale configuration setting where a set-in-stone setting can be passed using the configuration:

https://github.com/plone/volto/blob/master/src/components/manage/Blocks/Teaser/utils.js#L11

@sneridagh
Copy link
Member

sneridagh commented Apr 21, 2023

I'm also with @davisagli this has to be solved in the image handling itself, not at the block level.

@erral we cannot put something in core, that we are not going to need in the nearest future, then remove it (since it will be breaking).

As the other said, some efforts are in progress towards this already, probably during the upcoming Beethoven sprint the community will make it happen, so we won't need this.

Sorry @cihanandac we can't accept this right now, but thanks for your contribution! Maybe you can help pushing the improve images PR. You can always create an add-on out of it if you need it in your projects.

@erral
Copy link
Member

erral commented Apr 21, 2023

Sorry I don't agree.

I don't see that we are going to decide on the img srcset in the near future.

I think we need some flexibility on the selection of the scale size used in the teaser. I am not talking about rendering the correct image size depending on the resolution or browser window size.

Just being able to select the scale size, to be able to add this teaser in some other place where a smaller or bigger image can be required. I think that the current undocumented feature of setting the scale size at block level is not enough for this.

Perhaps the whole story is that I don't fully understand how the img srcset works, but as far as I can see it it calculates the full window width and not the container div, so it will not address my issue.

Anyway, these are my 2 cents.

@erral erral closed this Apr 21, 2023
@cihanandac
Copy link
Author

@sneridagh no worries, there are many more issues to contribute. I also agree that we should not add a feature that will be obsolete in the near future.
Thanks everyone for the reviews. I learned a lot in the process.

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

Successfully merging this pull request may close these issues.

Teaser block image scale size should be more configurable
5 participants