-
Notifications
You must be signed in to change notification settings - Fork 15
Specify sizes where we use the Image component #664
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
base: main
Are you sure you want to change the base?
Conversation
0890953 to
3b1616a
Compare
3b1616a to
5e3ef87
Compare
| className="item-image" | ||
| item={item} | ||
| imageComponent={PreviewImageComponent} | ||
| sizes="auto, (max-width: 940px) 100vw, 940px" |
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.
Grid listing. Get a large enough image for a single-column grid (default width = 940px, or viewport width if the viewport is smaller).
This is too big when there are multiple columns. Ideally we should set this based on the number of columns.
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.
Also note, auto is only supported by Chrome, so we use it but with a fallback for other browsers
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.
Okay I checked this with the machine, and locally. If this is set, the size required is 940px * 2 (High DPI, retina display). So
<img class="responsive" src="/content-types/image/@@images/image-2400-1eab29053998404b455be01b51b0904a.jpeg" width="2400" height="1708" srcset="
/content-types/image/@@images/image-32-92b79a539493d4afd7d1febce934a9c7.jpeg 32w,
/content-types/image/@@images/image-64-62ad570be19f6e98984f95a2d0cdf2ce.jpeg 64w,
/content-types/image/@@images/image-128-d97ecc65af26f132747d01c2e871cade.jpeg 128w,
/content-types/image/@@images/image-200-132bb6132f6b0c759471bc89170ff49a.jpeg 200w,
/content-types/image/@@images/image-400-c3dd6e0ede33e0ab1daa237d2440c88f.jpeg 400w,
/content-types/image/@@images/image-600-00c9d99fca8cfc80776e49c32a5eed41.jpeg 600w,
/content-types/image/@@images/image-800-35a5e5edfb86f634bb33543f5b5e2700.jpeg 800w,
/content-types/image/@@images/image-1000-62ebd981872459441555bb3ec85ae9cb.jpeg 1000w,
/content-types/image/@@images/image-1200-9d30ede1367936313209b56a5ce359a9.jpeg 1200w,
/content-types/image/@@images/image-1600-4527ead25ceffcaa0a295134666493f4.jpeg 1600w,
/content-types/image/@@images/image-2400-1eab29053998404b455be01b51b0904a.jpeg 2400w"
loading="lazy"
decoding="async"
alt="" image_field="image"
sizes="auto, (max-width: 940px) 100vw, 940px">The browser gets the scale above 1880px, which for our scales definition will be always the original size. In this case, the 2400 size.
| item={item} | ||
| showPlaceholderImage={true} | ||
| imageComponent={PreviewImageComponent} | ||
| sizes="auto, (max-width: 768px) 100vw, 220px" |
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.
Summary listing: images are full width on mobile, otherwise --card-listing-image-size (220px)
| alt="" | ||
| loading="lazy" | ||
| responsive={true} | ||
| sizes="auto, (max-width: 1440px) 100vw, 1440px" |
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.
The default slider variation. Layout width (--layout-container-width = 1440px) unless the viewport is smaller.
| alt="" | ||
| loading="lazy" | ||
| responsive={true} | ||
| sizes="auto, (max-width: 768px) 100vw, 600px" |
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.
Simple slider variation. No more than 600px on desktop, full width on mobile.
| item={!data.overwrite ? href : { ...href, ...filteredData }} | ||
| image={data.overwrite ? image : undefined} | ||
| imageComponent={Image} | ||
| sizes="auto, (max-width: 940px) 100w, 940px" |
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.
Teaser block. default width (940px) on desktop, or full width if the viewport is smaller.
This is too big for most cases (anything except a teaser in a one-column grid). Ideally we should set sizes based on the size of the grid that the teaser is inside, but I'm not sure how to get that here.
| imageField="image" | ||
| alt="" | ||
| responsive={true} | ||
| sizes="(max-width: 940px) 100vw, 940px" |
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.
Image content type view. Default width (940px), or full width if the viewport is smaller.
We don't use auto here because it doesn't do anything for images that aren't using loading="lazy"
|
@davisagli I am confused here, didn't we agree that you were going to explore to improve the inclusion of the original size given a max size value somewhere rather that touching everywhere? |
|
@davisagli I think that we are breaking the sizes= by putting auto with different value
if I remove the others and only kept auto it loads correct in chrome and even less scale in firefox and safari. |
|
@davisagli we can track the height of image wrapper also and set the size dirrectly to image tag in image component of volto. Pleas also consider this approach. |
@sneridagh As far as I can tell, we probably need both. I explained in https://gitlab.kitconcept.io/kitconcept/distribution-kitconcept-intranet/-/issues/167#note_23216
@iFlameing https://developer.mozilla.org/en-US/docs/Web/HTML/Reference/Elements/img says it is allowed to use
@iFlameing That won't work because we can't measure the height of the img until after the page has rendered, but at that time it is too late and the browser already started downloading the original image from |
|
@davisagli should we move this forward, then? As first step, we will have the sizes in place. |
|
@sneridagh Well, it would be nice if we could set sizes smaller for teasers in a grid. |
|
But it's also ok to go ahead with the easy fixes and add more later. |
5e3ef87 to
a3ed89c
Compare
* main: (74 commits) Release 8.0.0a7 Use Volto 19a14 (#733) Fix style of unauthorized Page (#732) Refactoring to TS some components (#731) fix accordion arrows (#728) Update RAC (#727) Release 8.0.0a6 Missing @kitconcept/volto-bm3-compat as dep (#726) Release 8.0.0a5 Identify intranet header with a className in `header header-intranet`… (#724) Registry color definitions support for ColorSwatch widget. (#723) Native namespaces (#716) Sticky menu fixes (#691) More addons version normalize (#717) Release 8.0.0a4 Fixed slider config in case it's not present (#714) Better `install.md` section (#710) Update dsgvo to 3.x series (#712) Release 8.0.0a3 Introducing recoverable error boundaries for blocks (#708) ...
|
Moved the sizes of the slider to the add-on: kitconcept/volto-slider-block#58 |
|
@davisagli let's move this one and make it happen for Volto 19. |
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.
@davisagli let's give a final look and merge it. I guess we should not wait for the other PRs to be merged, but it would be nice to keep them together.
@sneridagh This is a rough draft so I can add some comments about specific issues we need to consider.