helpscout / hsds-react

Help Scout Design System (HSDS) โ€” React Component Library
MIT License
86 stars 17 forks source link

Don't CSS reset img and svg width and height #1063

Closed justinwolfe closed 2 years ago

justinwolfe commented 2 years ago

Problem/Feature

The CSS reset seems to be working great and didn't break anything.....jk ๐Ÿ˜… ๐Ÿ˜น ! Here's what the Messages blank slate looked like for a little while (now fixed):

image

The CSS reset blew things up because the height and width got passed as props to the Image component, which means that they're set as attribute properties directly on the image. i.e. this

  function renderExampleMessageImage() {
    return (
      <Fragment>
        <ExampleMessageImageUI
          height="412"
          src={getImagePath('example-message.png')}
          title="Example Message"
          width="315"
        />
      </Fragment>
    )
  }

Renders as this HTML:

<img height="412" src="http://localhost:9020/images/messages/blank-slate/example-message.png" title="Example Message" width="315" data-cy="Image" class="Imagecss__ImageUI-egra31-0 eSUNsm c-Image sc-fzpjYC fOXZih">

Because width and height attributes in images and iframes have (thanks for the link @Charca) the weakest specificity of all, that meant that the reset was resetting them to initial, letting them balloon in size ๐Ÿšซ !

This was an issue that @ryan-mulrooney noticed in Message and a few other places in hs-app when QAing the original reset PR and led to adding the cssReset prop to toggle it on and only doing so in hs-app-ui and not hs-app...which works great except we didn't totally connect the dots that Messages is in hs-app-ui now so the prop didn't prevent this case from happening.

Solution

There was an easy immediate fix for this particular case that @jakubjanczyk took, which was just to move the width and height declarations out of props passed to the component and into CSS rules in the Styled Component (since the SC class specificity would be higher then and the issue wouldn't happen).

So we're okay now, but @Charca noted:

for the future, I'm wondering whether we could not reset width and height for elements that could take a presentational attribute (img, iframe, svg, etc) to prevent cases like this one? Although now that we're using the css reset, cases like this one will be much more obvious in the future, so we probably don't have to worry too much about it.

That makes sense to me, so this PR makes that change!

Guidelines

Make sure the pull request: