Skip to content

Demo Site: Add createImageSizes helper #3804

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

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

stekalt
Copy link
Contributor

@stekalt stekalt commented Apr 24, 2025

Description

The function createImageSizes is implemented to generate the sizes attribute string for Next.js images based on the breakpoints in the project. The sizes attribute is used by the browser to decide which image from the srcset to load depending on the viewport width. If not set, it defaults to 100vw.

Acceptance criteria

Screenshots/screencasts

Bildschirmaufnahme.2025-04-24.um.15.25.08.mov

Further information

@stekalt stekalt self-assigned this Apr 24, 2025
@auto-assign auto-assign bot requested a review from johnnyomair April 24, 2025 13:42
@stekalt stekalt force-pushed the add-createNextImageSizes-function branch from 5f93d98 to dfb5c9f Compare April 28, 2025 13:52
@stekalt stekalt force-pushed the add-createNextImageSizes-function branch from 2ba9767 to 13838b5 Compare April 30, 2025 09:03
@stekalt stekalt force-pushed the add-createNextImageSizes-function branch from 13838b5 to f0da793 Compare May 5, 2025 09:03
@stekalt stekalt force-pushed the add-createNextImageSizes-function branch from f0da793 to e684f5f Compare May 5, 2025 09:05
@johnnyomair johnnyomair self-requested a review May 7, 2025 07:42
@johnnyomair johnnyomair added the needs-starter-pr Change in Demo that needs to be changed in Starter as well. label May 13, 2025
@johnnyomair johnnyomair changed the title Demo Site: Add createNextImageSizes helper Demo Site: Add createImageSizes helper May 13, 2025
johnnyomair
johnnyomair previously approved these changes May 13, 2025
thomasdax98
thomasdax98 previously approved these changes May 13, 2025

type BreakpointWidths = { xs: string | number } & Partial<Record<keyof Theme["breakpoints"], string | number>>;

export function createImageSizes(breakpointWidths: BreakpointWidths) {
Copy link
Member

Choose a reason for hiding this comment

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

@johnnyomair
I'm not sure a "fake" xs breakpoint (that is not a breakpoint) is a good idea. Why not how it was originally implemented, using a second argument?

Copy link
Collaborator

Choose a reason for hiding this comment

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

My rationale for this was that the size for the mobile viewport is more visible. I think this

<DamImageBlock sizes={createImageSizes({ xs: "100vw", md: "50vw" }) />

is more understandable than

<DamImageBlock sizes={createImageSizes({ md: "50vw" }, "100vw") />

or even

<DamImageBlock sizes={createImageSizes({ md: "50vw" }) />

(default 100vw).

I'm fine with changing it back though. I'd drop the default value because devs then need to think about the size for mobile (might not be 100vw all the time).

Copy link
Member

Choose a reason for hiding this comment

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

yes, the default value can cause issues, and un-named arguments are hard to read.

Other alternatives:

simply rename xs to default (because that's what it is):
<DamImageBlock sizes={createImageSizes({ default: "100vw", md: "50vw" }) />

or, move the arguments into an options object:
<DamImageBlock sizes={createImageSizes({ default: "100vw", breakpoints: { md: "50vw" } }) />

Copy link
Collaborator

Choose a reason for hiding this comment

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

Both alternatives are okay for me. @stekalt what do you think? Which one would you prefer to use on a day-to-day basis?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I liked the approach using xs – for me, it made it clear that the breakpoints follow the min-width principle (even if xs is just a placeholder for the default case).

Out of the alternatives I think the options object provides a clearer separation between the default value and the breakpoints, but in working with it I would prefer the first alternative, as it is simply shorter to write:

<DamImageBlock sizes={createImageSizes({ default: "100vw", md: "50vw" }) />

Copy link
Member

@nsams nsams May 20, 2025

Choose a reason for hiding this comment

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

it made it clear that the breakpoints follow the min-width principle

strictly speaking, this xs is not a breakpoint and does not have a min-width.

Like in the normal styling, you write mobile-first (=default) styles and on top of that min-width breakpoints.

You should even remove the (min-width: 0px), according to mdn:

  1. A media condition This must be omitted for the last item in the list.

Copy link
Collaborator

Choose a reason for hiding this comment

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

strictly speaking, this xs is not a breakpoint and does not have a min-width.

UX calls mobile XS and uses this in their Figma designs though.

We could change the argument to viewportWidths, then xs would be valid.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Like in the normal styling, you write mobile-first (=default) styles and on top of that min-width breakpoints.

Yes we do, but most sizes-examples (also on mdn) are using the max-width approach. This is why I pointed out the min-width principle in the argumentation of my opinion.

You should even remove the (min-width: 0px), according to mdn:

I removed the (min-width: 0px) from the default item.

Copy link
Member

Choose a reason for hiding this comment

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

If you think xs makes more sense than default, it should be fine for me 👍

Copy link
Collaborator

Choose a reason for hiding this comment

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

@stekalt please use the naming you prefer – both @nsams and I won't use it in projects very often, if ever 😁

@fraxachun fraxachun removed their request for review May 15, 2025 07:53
@dkarnutsch dkarnutsch removed their request for review May 19, 2025 09:35
@stekalt stekalt dismissed stale reviews from thomasdax98 and johnnyomair via 459c69b May 21, 2025 06:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-starter-pr Change in Demo that needs to be changed in Starter as well.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants