GoogleForCreators / web-stories-wp

Web Stories for WordPress
https://wp.stories.google
Apache License 2.0
768 stars 178 forks source link

Design System: Create reusable image component #11806

Closed spacedmonkey closed 2 years ago

spacedmonkey commented 2 years ago

Feature Description

There are a number places where image tags are used in the code based. To ensure that image would with cross origin images works as expected, the crossOrigin attributes has to be added. This means a number of places where crossOrigin attribute has added manually to the tag. decoding="async" should also be added to most image tags.

Create image component in design package and reuse it.

Alternatives Considered

Additional Context

swissspidy commented 2 years ago

It is worth noting that crossorigin=anonymous is not always needed nor desired. So we need to be careful about that and not just adding this by default.

In our case it's only relevant for things like colorthief, poster generation, and displaying media when cross-origin isolation is enabled.

See also https://developer.mozilla.org/en-US/docs/Web/HTML/Attributes/crossorigin

swissspidy commented 2 years ago

@sblinde @merapi Curious to hear your thoughts on this as pod leads.

As per the above, I have some reservations about just adding crossorigin=anonymous by default. It's not something we need in the dashboard for example.

Second, re-sharing this comment from #11839:

When thinking about introducing such a base image component it's a good opportunity to think about the goals we want to achieve and the features we'd expect from it, like for example lazy loading or automatic support for displaying a base color as background.

sblinde commented 2 years ago

@sblinde @merapi Curious to hear your thoughts on this as pod leads.

As per the above, I have some reservations about just adding crossorigin=anonymous by default. It's not something we need in the dashboard for example.

Agreed, in many cases I don't think it's necessary, especially when an image is not cross origin, so not having it as a default is the right path to me as well.

Second, re-sharing this comment from #11839:

When thinking about introducing such a base image component it's a good opportunity to think about the goals we want to achieve and the features we'd expect from it, like for example lazy loading or automatic support for displaying a base color as background.

Seconding as well with this. If there were a certain use case of an image that, say, was for generating a thumbnail in a certain size or with certain support (like say, an image with a visibly written caption or something to that effect), I'd be more in favor of it, something that inherently makes the usage easier for everyone involved. Sometimes I feel like creating a component for things that are not specific style-driven, like basic HTML elements is doing too much abstraction that isn't necessary, especially when this is something that would be controlled on the engineer's end anyway. I don't see what would be gained from this approach given my context here.

spacedmonkey commented 2 years ago

This ticket comes out of the issue, that while working on hotlinking, there were examples where crossorigin attribute was missed. There is correctly no way to easily see / test that all images in the editor have this attribute. Maybe a better solution is a just some kind of lint.

But forcing all images to have these attributes, does have value and is worth doing, IMO.