Skip to content

Conversation

@dcmouyard
Copy link
Member

This PR adds responsive images to Storybook and updates components to use them.

Copy link
Collaborator

@kmonahan kmonahan left a comment

Choose a reason for hiding this comment

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

Another one that really needs a "Needs discussion" status.

I wholeheartedly agree that responsive images > non-responsive images. But a lot of the work here seems to be around creating or modifying custom Twig templates for images, and I don't think we should be doing that. We should use what Drupal provides us, which is a responsive image using img or picture depending on your Responsive Image style settings.

In the CTA, for example, the media now comes from a series of nested Twig templates and render functions that, to me, falsely creates the impression that I can pass in other modifiers to my image or modify the responsive image template. Before, it was clear that you were going to be handed an image element to slot in where needed.

I might be overthinking things here and maybe these are just meant as ways to easily add responsive images as placeholders in Storybook without having to handwrite a bunch of image sizes every time. If that's the case, it changes my opinion and I'm on board with it. What I want to avoid is something like we have with links, where we constantly have to tear apart the Drupal render array to feed it to our own template.

But like I said, needs discussion! I'll tag other people here for more perspectives.

@coreylafferty
Copy link
Member

Dan/Tommy and I just talked about this and I think a it might address the concerns KJ raised if we:

  • Got rid of the Twig files and moved everything in the yml/story files
  • Renamed the folder (and possibly the files) to something like "drupal-responsive-image-styles" to more clearly communicate these files are for figuring out or emulating your image styles in Storybook, but aren't to be used as components.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants