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

[@astrojs/image] flatten background only if alpha channel isn't supported #4800

Merged
merged 5 commits into from
Sep 20, 2022

Conversation

obennaci
Copy link
Contributor

@obennaci obennaci commented Sep 19, 2022

Changes

Following up #4642, these changes make sure we keep transparency unless the output format doesn't support alpha channel.

I also took the liberty to allow rgba in ColorDefinition, makes it possible to use transparency when combined with #4438 without TS errors.

Testing

The existing tests assume the background is flattened every time. I went ahead and specified jpeg as output format for existing fixtures.

Docs

/cc @withastro/maintainers-docs

@obennaci obennaci requested a review from a team as a code owner September 19, 2022 08:38
@changeset-bot
Copy link

changeset-bot bot commented Sep 19, 2022

🦋 Changeset detected

Latest commit: 203aa8c

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@astrojs/image Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions github-actions bot added the pkg: integration Related to any renderer integration (scope) label Sep 19, 2022
Copy link
Contributor

@tony-sull tony-sull left a comment

Choose a reason for hiding this comment

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

Nice! Looks good to me, I'll make sure the docs mark this as a sharp-only feature once the WASM work is merged in

@tony-sull tony-sull merged commit ea37de8 into withastro:main Sep 20, 2022
@astrobot-houston astrobot-houston mentioned this pull request Sep 20, 2022
@beeb
Copy link
Contributor

beeb commented Jan 30, 2023

Hey there! I initially added the option to replace the background with a solid color in @astrojs/image (#4642).
The intent was to allow to replace the transparent background on an image regardless of the output format. With this PR, formats that support transparency cannot anymore be transformed to have a solid background and no alpha channel.

The initial behavior (allowing replacing alpha with color for all formats) is useful because such images have a smaller filesize. This is why I added this feature and how I'm using it in my projects (I have a source of truth as transparent PNG and generate an optimized image in webp without alpha at build time). Since webp supports transparency, this PR increased the size of my assets.

If that's OK, I will soon make a PR that restores the initial behavior, assuming that when the user passes a background property to the component, they want to set the background to a specific color.
I'll also take the opportunity to fix the tests where the rgba variants don't use rgba in the fixture at the moment (which lead to a typescript warning)

@beeb
Copy link
Contributor

beeb commented Jan 30, 2023

Pinging @tony-sull to see if that would be ok. (here is a preview of the changes beeb@7a78ce6 )

@tony-sull
Copy link
Contributor

@beeb changing the current behavior would be a breaking change, I'd definitely recommend starting up an RFC discussion before you dive too far into a PR to get some feedback on the idea 🙌

Erika opened a discussion not long ago to kickstart a cleanup of @astrojs/image, that might actually be a good place to add this feedback

Personally I've always been torn on exposing so many sharp-specific props through our components - a general "make @astrojs/image easier to use" RFC might be the right time to revisit whether we even want all the extra complexity of image transformations beyond the basics of dimensions and image format 🤔

@beeb
Copy link
Contributor

beeb commented Jan 30, 2023

Cheers @tony-sull , I'll pitch in on the discussion thread. Note that this PR #4800 was a breaking change to the behavior I implemented. But it was never documented as such.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: integration Related to any renderer integration (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants