Skip to content

Conversation

@kyledurand
Copy link
Member

@kyledurand kyledurand commented Oct 26, 2022

Part of #7592

API playground
import React from 'react';

import {Page, Columns} from '../src';

export function Playground() {
  return (
    <Page title="Playground">
      <Columns columns={7} spacing={{xs: '1', sm: '2', md: '3', lg: '4'}}>
        {genChildren(23)}
      </Columns>

      <hr />

      <Columns columns={['oneThird', 'twoThirds']}>{genChildren(5)}</Columns>

      <hr />

      <Columns columns={{xs: 3, sm: 4, md: 5}}>{genChildren(6)}</Columns>

      <hr />

      <Columns columns={{xs: ['twoThirds', 'oneThird']}}>
        {genChildren(6)}
      </Columns>

      <hr />

      <Columns
        columns={{xs: ['oneHalf', 'oneHalf'], lg: ['oneThird', 'twoThirds']}}
      >
        {genChildren(6)}
      </Columns>

      <hr />

      <Columns columns={{xs: ['oneHalf', 'oneHalf']}}>{genChildren(6)}</Columns>
    </Page>
  );
}

function genChildren(count: number) {
  const background =
    'repeating-linear-gradient(45deg, var(--p-background), var(--p-background) 5px, var(--p-decorative-four-surface) 5px, var(--p-decorative-four-surface) 10px';
  return Array.from({length: count}).map((_, index) => (
    <div key={index} style={{background}}>
      {index}
    </div>
  ));
}

@github-actions
Copy link
Contributor

github-actions bot commented Oct 26, 2022

size-limit report 📦

Path Size
polaris-react-cjs 209.28 KB (+0.03% 🔺)
polaris-react-esm 135.65 KB (+0.02% 🔺)
polaris-react-esnext 191.23 KB (+0.02% 🔺)
polaris-react-css 41.72 KB (+0.01% 🔺)

@kyledurand kyledurand force-pushed the proto_columns-niceties branch from 8122cdf to 8c7f14a Compare October 26, 2022 18:35
@kyledurand kyledurand force-pushed the proto_columns-niceties branch from 8c7f14a to e628ec8 Compare November 2, 2022 18:25
@kyledurand kyledurand changed the title Add oneThird, oneHalf, twoThirds options to column types Allow single value for columns. Add oneThird, oneHalf, twoThirds array options Nov 2, 2022
@kyledurand kyledurand force-pushed the proto_columns-niceties branch from 73a5858 to d8f14bf Compare November 2, 2022 18:29
? `var(--p-space-${spacing?.xl})`
: undefined,
} as React.CSSProperties;
} as unknown as React.CSSProperties;
Copy link
Member Author

Choose a reason for hiding this comment

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

Might have to collab with @aaronccasanova the TS 🧙‍♂️ on this. Fine for now though and we can iterate as we add similar functionality to other responsive components

Copy link
Contributor

Choose a reason for hiding this comment

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

We can use the getResponsiveProps() function here to replace lines 48-62

...getResponsiveProps('columns', 'spacing', 'space', spacing || '4'),

Copy link
Member Author

Choose a reason for hiding this comment

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

That won't quite solve this problem but it should clean up the code. I was planning on cleaning this stuff up after if it was blocking you but if not then I can try fixing it later today

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not blocked by this, so all good!

@kyledurand kyledurand requested review from aveline and laurkim November 2, 2022 18:31
@kyledurand kyledurand self-assigned this Nov 2, 2022
@kyledurand kyledurand marked this pull request as ready for review November 2, 2022 18:31
Copy link
Contributor

@aveline aveline left a comment

Choose a reason for hiding this comment

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

I think we should also allow passing in widths non responsively the way we do numbers. Like <Columns columns={['oneThird', 'twoThirds']}>

? `var(--p-space-${spacing?.xl})`
: undefined,
} as React.CSSProperties;
} as unknown as React.CSSProperties;
Copy link
Contributor

Choose a reason for hiding this comment

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

We can use the getResponsiveProps() function here to replace lines 48-62

...getResponsiveProps('columns', 'spacing', 'space', spacing || '4'),

@kyledurand
Copy link
Member Author

I think we should also allow passing in widths non responsively the way we do numbers. Like <Columns columns={['oneThird', 'twoThirds']}>

That won't help you build the layout component though 🤔

@aveline
Copy link
Contributor

aveline commented Nov 2, 2022

Generally speaking any props that have responsive support should also the same values without having to include them in the responsive object. So I could do something like

columns=2 or columns={{xs: 2}}

Right now it seems inconsistent that you can do the above but only columns={{xs:['oneThird', 'twoThirds']}} and not columns={['oneThird', 'twoThirds']}

@kyledurand kyledurand force-pushed the proto_columns-niceties branch from 7dada5f to 81c7ced Compare November 2, 2022 20:49
@kyledurand kyledurand requested a review from aveline November 2, 2022 20:50
@kyledurand kyledurand marked this pull request as draft November 2, 2022 20:54
@kyledurand
Copy link
Member Author

I'm not sure how I feel about this API anymore so I'm moving it back to draft. Going to keep tinkering with it but I also have higher priorities for the rest of the week

@laurkim
Copy link
Contributor

laurkim commented Feb 7, 2023

Hey @kyledurand, should we revisit this work or close this PR? Some of the iterations on documentation/guidance for this component includes this functionality so I'd like to verify whether we want to support this before updated guidance ships.

@aveline
Copy link
Contributor

aveline commented Feb 13, 2023

I think this is still important. the gap prop "Accepts a spacing token or an object of spacing tokens for different screen sizes."

It would make sense and be consistent for the columns prop to do the same, accept a value for columns or an object of values for different screen sizes.

@SeanyB SeanyB assigned aveline and unassigned kyledurand Feb 14, 2023
@aveline
Copy link
Contributor

aveline commented Feb 15, 2023

Replaced with #8387

@aveline aveline closed this Feb 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants