[Stack] Fix min-width issue on Stack.Item #2273

dleroux · 2019-10-10T14:08:30Z

WHY are these changes introduced?

In fixing this issue #1707 we removed min-width thinking it wasn't needed. Turns out it is.

WHAT is this pull request doing?

Simply bringing back the min-width so that the flex-items takes the space it requires.

We do still have an issue where if textOverflow: 'ellipsis', is used on the child of the flex item, overflow is a little off. In which case its probably best to write flexbox for this edge case.

This fixed the bigger issue of the column not appearing.

How to 🎩

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

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

import React from 'react';
import {AppProvider, Page, Layout, Card, Stack} from '../src';

export function Playground() {
  return (
    <Page title="Playground" narrowWidth>
      <Layout sectioned>
        <Card title="Broken 1" sectioned>
          <Stack>
            <div
              style={{
                overflow: 'hidden',
                whiteSpace: 'nowrap',
                textOverflow: 'ellipsis',
              }}
            >
              This should cause hello to wrap and also make sure that my text
              does not overflow my container. The ellipsis is spilling into the
              margin of the card however.
            </div>
            <div>
              <p>This line should wrap</p>
            </div>
          </Stack>
        </Card>
        <Card title="Broken 2" sectioned>
          <Stack alignment="center" wrap={false}>
            <div
              style={{
                overflow: 'hidden',
                whiteSpace: 'nowrap',
                textOverflow: 'ellipsis',
              }}
            >
              This should cause hello to still be visible and my content to be
              overflow hidden when I reach my container width. However, hello is
              hidden entirely and the ellipsis is spilling into the card margin.
            </div>
            <div>
              <p>hello world</p>
            </div>
          </Stack>
        </Card>
      </Layout>
    </Page>
  );
}

🎩 checklist

Tested on mobile
Tested on multiple browsers
Tested for accessibility
Updated the component's README.md with documentation changes
Tophatted documentation changes in the style guide
For visual design changes, pinged one of @ HYPD, @ mirualves, @ sarahill, or @ ry5n to update the Polaris UI kit

github-actions · 2019-10-10T14:11:17Z

💦 Potential splash zone of changes introduced to src/**/*.tsx in this pull request:

Files modified	3
Files potentially affected	29

Details

All files potentially affected (total: 29)

📄 UNRELEASED.md (total: 0)

Files potentially affected (total: 0)

🎨 src/components/Stack/Stack.scss (total: 29)

Files potentially affected (total: 29)

📄 src/components/TextField/README.md (total: 0)

Files potentially affected (total: 0)

This comment automatically updates as changes are made to this pull request.
Feedback, troubleshooting: open an issue or reach out on Slack in #polaris-tooling.

dleroux · 2019-10-10T14:23:46Z

Looks like in some case this is touching the edge. On Percy, but also on ios.

dfmcphee · 2019-10-10T14:29:45Z

Looks like in some case this is touching the edge. On Percy, but also on ios.

Yeah, that's interesting. It looks like that example is a little wonky though. It uses nested stacks to do something that should be handled by a FormLayout instead.

LauraAubin · 2019-10-10T14:31:57Z

src/components/Stack/Stack.scss


 .Item {
  flex: 0 1 auto;
+  min-width: 0;


When the stack width is smaller than the contents, the contents overflow each other. Maybe we could make them wrap to the next line? Not sure if this would introduce even more edge cases though

Because this is an abstraction of Flexbox we might just want to leave it as is. Wrapping is left up to the props which should generate the right flexbox rules.

dleroux · 2019-10-10T20:41:01Z

There's still some weirdness happening with this when white space nowrap is set on a children. Overall I think this is better than we have today since the column is not showing up at all. I'm thinking if you are happy with the top hat Dom, we ship this for now.

dfmcphee

I think this is the better option of the two. 🚢

dleroux force-pushed the fixing-stack branch 2 times, most recently from 21cff9e to afabb30 Compare October 10, 2019 14:15

dleroux requested review from LauraAubin and dfmcphee October 10, 2019 14:17

dleroux changed the title ~~[WIP][Stack] Fix min-width issue on Stack.Item~~ [Stack] Fix min-width issue on Stack.Item Oct 10, 2019

dfmcphee approved these changes Oct 10, 2019

View reviewed changes

dleroux changed the title ~~[Stack] Fix min-width issue on Stack.Item~~ [WIP][Stack] Fix min-width issue on Stack.Item Oct 10, 2019

dleroux changed the title ~~[WIP][Stack] Fix min-width issue on Stack.Item~~ [Stack] Fix min-width issue on Stack.Item Oct 10, 2019

dleroux changed the title ~~[Stack] Fix min-width issue on Stack.Item~~ [wip][Stack] Fix min-width issue on Stack.Item Oct 10, 2019

dleroux requested a review from dfmcphee October 10, 2019 14:24

LauraAubin reviewed Oct 10, 2019

View reviewed changes

bringing back min-width

b2dcbe1

dleroux force-pushed the fixing-stack branch from 5d0a639 to b2dcbe1 Compare October 10, 2019 19:59

dleroux changed the title ~~[wip][Stack] Fix min-width issue on Stack.Item~~ [Stack] Fix min-width issue on Stack.Item Oct 10, 2019

dfmcphee approved these changes Oct 11, 2019

View reviewed changes

dleroux merged commit a1e2d51 into master Oct 11, 2019

dleroux deleted the fixing-stack branch October 11, 2019 18:48

tmlayton temporarily deployed to production October 16, 2019 00:32 Inactive

tmlayton temporarily deployed to patch October 17, 2019 22:38 Inactive

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[Stack] Fix min-width issue on Stack.Item #2273

[Stack] Fix min-width issue on Stack.Item #2273

Uh oh!

dleroux commented Oct 10, 2019 •

edited

Loading

Uh oh!

github-actions bot commented Oct 10, 2019 •

edited

Loading

Files potentially affected (total: 0)

Files potentially affected (total: 29)

Files potentially affected (total: 0)

Uh oh!

dleroux commented Oct 10, 2019

Uh oh!

dfmcphee commented Oct 10, 2019

Uh oh!

LauraAubin Oct 10, 2019

Uh oh!

dleroux Oct 11, 2019

Uh oh!

dleroux commented Oct 10, 2019

Uh oh!

dfmcphee left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

[Stack] Fix min-width issue on Stack.Item #2273

[Stack] Fix min-width issue on Stack.Item #2273

Uh oh!

Conversation

dleroux commented Oct 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

WHY are these changes introduced?

WHAT is this pull request doing?

How to 🎩

🎩 checklist

Uh oh!

github-actions bot commented Oct 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Files potentially affected (total: 0)

Files potentially affected (total: 29)

Files potentially affected (total: 0)

Uh oh!

dleroux commented Oct 10, 2019

Uh oh!

dfmcphee commented Oct 10, 2019

Uh oh!

LauraAubin Oct 10, 2019

Choose a reason for hiding this comment

Uh oh!

dleroux Oct 11, 2019

Choose a reason for hiding this comment

Uh oh!

dleroux commented Oct 10, 2019

Uh oh!

dfmcphee left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dleroux commented Oct 10, 2019 •

edited

Loading

github-actions bot commented Oct 10, 2019 •

edited

Loading