Skip to content

Conversation

jesstelford
Copy link
Contributor

@jesstelford jesstelford commented Mar 13, 2023

WHY are these changes introduced?

It's currently possible to add <Divider />s manually to stacks;

<AlphaStack>
  <Box />
  <Divider />
  <Box />
  <Divider />
  <Box />
</AlphaStack>

However, this comes with two big caveats:

  1. No enforcement of consistency for all dividers.
    Human error could easily lead to rendering dividers with different borderStyle props, especially if those components are far apart / complex. It's even possible to accidentally forget a divider when there should be one:
    <AlphaStack gap="4">
      <Box />
      <Divider borderStyle="dark" />
      <Box />
      <Divider borderStyle="dark" />
      <Box />
      <Divider borderStyle="dark" />
      <Box />
      <Box />
      <Divider borderStyle="divider" />
      <Box />
    </AlphaStack>
    (sandbox link)
  2. Requires the parent/rendering component to know where to insert dividers.
    When rendering lists of items within a stack, it requires custom logic to insert dividers between each element:
    const items = ["foo", "bar", "baz", "zip"];
    return (
      <AlphaStack gap="8">
        {items.map((item, index) => {
          if (index === 0) {
            return item;
          }
          return (
            <>
              <Divider />
              {item}
            </>
          );
        })}
      </AlphaStack>
    );
    (sandbox link)

By enabling automatic insertion of dividers, both of these caveats become non-issues; The parent can defer where to render dividers to the <Stack> component, and consistency is enforced by the <Stack> component:

<AlphaStack withDivider>
  <Box />
  <Box />
  <Box />
</AlphaStack>
const items = ["foo", "bar", "baz", "zip"];
return (
  <AlphaStack withDivider>
    {items.map(item => <Box>{item}</Box>)}
  </AlphaStack>
);

WHAT is this pull request doing?

Added the withDivider prop to <AlphaStack>:

  • Default: false
  • When false, current beahviour is identical
  • When true, renders a <Divider /> between each child element
  • Can provide a string which sets the divider style (uses the type from Divider's borderStyle prop).

✅ Added Storybook story, tests, and documentation.

Prior work

@jesstelford jesstelford marked this pull request as draft March 13, 2023 04:13
@github-actions
Copy link
Contributor

github-actions bot commented Mar 13, 2023

size-limit report 📦

Path Size
polaris-react-cjs 221.42 KB (+0.04% 🔺)
polaris-react-esm 140.8 KB (+0.01% 🔺)
polaris-react-esnext 197.58 KB (+0.02% 🔺)
polaris-react-css 43.08 KB (-0.03% 🔽)

@jesstelford jesstelford changed the title AlphaStack accepts withDivider to insert Dividers between stack children [AlphaStack] Add withDivider prop to insert Dividers between stack children Mar 13, 2023
@jesstelford jesstelford marked this pull request as ready for review March 13, 2023 06:45
@kyledurand
Copy link
Member

@jesstelford can we copy / paste some of the slack conversation into this PR for more context / opposing views?

Another thing I don't think was mentioned in slack was that the Divider component takes a prop for border style which means if we incorporate it into the stack component we'd (probably, eventually) need to accommodate those props, which means more prop drilling, something that's currently adding complexity across the board and hindering us in making updates quickly

@jesstelford
Copy link
Contributor Author

jesstelford commented Mar 23, 2023

the Divider component takes a prop for border style

Yep, that's already handled in this PR (mirrors the implementation in SEEK's Braid):

Can provide a string which sets the divider style (uses the type from Divider's borderStyle prop).


more context / opposing views

Here are the (anonymised) comments from when this was proposed:

I am a bit concerned with adding the withDivider prop to AlphaStack because it blurs the boundaries between the responsibilities for our layout components. I do agree that it’s a bit of a pain to have to repeatedly add the Divider to AlphaStack but I think adding the prop can become a point of tension, as it may encourage one off props on our layout primitives that negate the purpose of our primitives being flexible with tightly contained responsibilities

I feel the same concerns as [above]. We want to optimize for clear concise layout components that are able to do the things you need to do through composition.

the use case for stack with a divider might be a good opportunity to explore creating composed components. At this point I think consumers could use examples and guidance around that.

@itwasmattgregg
Copy link
Contributor

There are a lot of designers internally who are building Cards with dividers. I think this may just be from how sectioned Cards used to work and now they are carrying it over but nevertheless, they are being designed. Opinion seems to be that dividers are not always evil, but overuse can be bad when simple vertical space and font sizes can create similar separation. I have seen enough examples though of Dividers in cards being used really well that this may actually merit a prop in Polaris. For now I've proposed a global component just within web to help with this. Allows composability but also ease of DX.

https://github.com/Shopify/web/pull/99497

Copy link
Contributor

Hi! We noticed there hasn’t been activity on this PR in a while. After 30 days, it will close automatically.

If it’s still relevant, or you have updates, comment and let us know. And don’t worry, you can always re-open later if needed.

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

Successfully merging this pull request may close these issues.

3 participants