Skip to content

Improvements for image sizes#8031

Merged
davisagli merged 3 commits into
donotaddoriginalimagetothesrcsetfrom
davisagli-sizes
Mar 23, 2026
Merged

Improvements for image sizes#8031
davisagli merged 3 commits into
donotaddoriginalimagetothesrcsetfrom
davisagli-sizes

Conversation

@davisagli

@davisagli davisagli commented Mar 19, 2026

Copy link
Copy Markdown
Member
  1. Get default width from a config setting. (To do later: use the same setting to generate the --default-container-width CSS property.)
  2. Set the sizes for a teaser block based on the number of grid columns.

1. Get default width from a config setting. (To do: use the same setting to generate the --default-container-width CSS property.)
2. Set the sizes for a teaser block based on the number of grid columns.
@davisagli davisagli requested review from pnicolli and sneridagh March 19, 2026 23:38

@sneridagh sneridagh left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I kind of like it! @davisagli it does replace #7655, right ?

@sneridagh

Copy link
Copy Markdown
Member

@davisagli Oh! I see now that it's based on the other!! 😂

Comment thread packages/volto/src/config/index.js Outdated
includeSiteTitle: false,
titleAndSiteTitleSeparator: '-',
},
defaultWidth: 940,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I am in favor of having the default container width in the settings because it helps with these image sizing issues, I would call it something like defaultContainerWidth to be more clear about what this is about, or nested in something like config.settings.layout.defaultWidth

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I updated it to use config.settings.layout.defaultContainerWidth

const TeaserView = (props) => {
return <TeaserBody {...props} />;
return (
<GridContext.Provider value={1}>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I understand that this is here because a teaser that is not in a grid will need a provider, right? Isn't this a problem when the teaser is actually inside a grid, where this will override the provider coming from the grid?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@pnicolli @sneridagh Yeah, I was thinking about this wrong. Yes, this was incorrectly overriding the provider from the grid. But we don't need a provider here, because the context has a default value of 1.

Looking into this I realized some other things:

  1. The image width is different for a standalone teaser (it's side by side with the text, so about half the width) and a teaser in a 1-column grid (there it's the full width of the container). I adjusted to handle this.
  2. The max-width for the first part of the sizes should be based on the breakpoint where images become full viewport-width, which is not necessarily the same as the default container width. So I added a tabletBreakpoint setting as well.
  3. The tablet breakpoint and default container width in the pastanaga theme are not 940px. So I set them to match the theme variables.

@davisagli davisagli merged commit 96ba8d7 into donotaddoriginalimagetothesrcset Mar 23, 2026
76 of 77 checks passed
@davisagli davisagli deleted the davisagli-sizes branch March 23, 2026 23:37
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.

3 participants