[Stack] Remove margin from vertical

[solonaarmstrong-zz]

WHY are these changes introduced?

Resolves #947

When using a vertical Stack the contents appear to break out of the container and "bleed" into surrounding padding when one of the children starts to wrap.

WHAT is this pull request doing?

Removes negative left margin from stack and left margin from item when vertical.

🤓 1024 - the gigabyte PR

How to 🎩

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

Copy-paste this code in playground/Playground.tsx:

import * as React from 'react';
import {Page, Card, Stack, Badge} from '../src';

interface State {}

export default class Playground extends React.Component<{}, State> {
  render() {
    return (
      <Page title="Playground">
        <Card sectioned>
          <Stack vertical>
            <div style={{backgroundColor: 'red'}}>Testing</div>
            <div>
              Lorem ipsum dolor sit amet, consectetur adipiscing elit. Vivamus
              sit amet rutrum tortor, quis condimentum lectus. Duis dui ligula,
              bibendum vel pharetra non, vulputate ut risus. Nam semper
              consequat nisl, sit amet consectetur purus vestibulum non. Duis
              molestie facilisis diam sit amet imperdiet. Praesent malesuada et
              nulla in mattis. Nullam aliquam luctus dui, id fermentum magna
              faucibus vitae. Ut ultrices dui vel lacus sodales, vel imperdiet
              quam facilisis. Curabitur dictum odio sit amet enim pretium
              consequat. Duis luctus sapien mattis sapien luctus tempor. Quisque
              vel purus quis orci dignissim pharetra sed sit amet turpis. Nam
              porttitor libero risus, ut facilisis lorem lobortis nec. Sed nec
              placerat sapien, eu auctor ex. Aliquam vel mattis neque. Integer
              pellentesque viverra fermentum.
            </div>
          </Stack>
        </Card>
        <br />
        <div style={{width: '200px'}}>
          <Card>
            <Stack vertical>
              <Badge>Badge with many words that wrap</Badge>
              <Badge>Badge with many words that wrap</Badge>
              <Badge>Badge with many words that wrap</Badge>
              <Badge>Badge with many words that wrap</Badge>
            </Stack>
          </Card>
        </div>
        <br />
        <div style={{width: '200px'}}>
          <Card>
            <Stack>
              <Badge>Badge</Badge>
              <Badge>Badge</Badge>
              <Badge>Badge</Badge>
              <Badge>Badge</Badge>
            </Stack>
          </Card>
        </div>
      </Page>
    );
  }
}

🎩 checklist

• Tested on mobile
• Tested on multiple browsers
• Tested for accessibility

[AndrewMusgrave]

Are we sure this is something we want to fix? I believe this is fixed in chrome v73, @tmlayton I heard you might have checked the canary release?

[mbaumbach]

I see this bug in Version 74.0.3705.0 (Official Build) canary (64-bit) on the Shopify Partner app specific summary page, which makes heavy use of vertical stacks, as well as the code sandbox provided in #947. Though I'm not sure if that's the appropriate browser version to be looking at and may still be fixed in v73.

[solonaarmstrong-zz]

Chrome v72

Chrome v73

Canary v74.0.3705.0

It was fixed and broke again?

[mbaumbach]

Weird, maybe it was hotfixed into v73 and hasn't been merged it into canary yet for some reason?

[mehdi-loup]

Just to add to the issue, I can see the problem with a button group in a card. The proposed changes would fix the problem.

My chrome version being 72.0.3626.109 (Official Build) (64-bit)

[AndrewMusgrave]

Sorry I got my versions wrong, it looks like Tim tested it on canary v74.

[solonaarmstrong-zz]

@AndrewMusgrave I have a screenshot for canary v74

[ry5n]

@solonaarmstrong Can you confirm this works (and doesn’t cause other issues) on supported browsers (including other desktop and mobile browsers than Chrome)? If so I think you’re right to encourage reviewers to look at this so we can ship a fix.

[solonaarmstrong-zz]

@ry5n Here are some more screenshots of other browsers. I don't have context on why the margin would be needed for vertical stacks otherwise. @dleroux apparently does? I might need to migrate my machine before being able to fire up VirtualBox. (No idea why it says Bell Wifi. I wish!)

Safari

iOS (Safari)

iOS (Firefox)

iOS (Chrome)

[mateus]

👋 Hey, just to catch up. Do we have any updates on this fix? I'm working on a redesigned version of the Payment Settings and we have this happening in quite a few pages. Is this issue not happening in many other places?

[chloerice]

👋 Hey, just to catch up. Do we have any updates on this fix? I'm working on a redesigned version of the Payment Settings and we have this happening in quite a few pages. Is this issue not happening in many other places?

Hey @mateus, I've tophatted these changes in web and the issue (in Chrome only) is still there. I believe it's only because the .vertical class is defined above the spacing prop classes and they are still adding unwanted margin. I'll pick up where Solona left off, stay tuned.

[chloerice]

Quick update - I tested the changes with the .vertical class moved below the spacing prop classes and it works as expected across browsers again 🎉

Before	After

[ry5n]

Sounds like this is working now.

[mateus]

Tested it by simply copying the proposed CSS over to the project and it seems to work as expected 👍 Thank you.