Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(grid): make subgrid work with grid offset #9803

Merged
merged 3 commits into from
Oct 13, 2021

Conversation

janhassel
Copy link
Member

Ref #9723 (comment)

Ensures that subgrid is supported corectly when the parent grid uses offset.

Changelog

Removed

  • Removed explicit col-end-n rules since the existing col-span-n and col-start-n classes together are already achieving the expected styling

Testing / Reviewing

Add the test case from the discussion as a story and verify the appearance / behaviour:

export const Test = () => {
  return (
    <Grid>
      <Column sm={4} md={{ span: 6, offset: 1 }} lg={{ span: 8, offset: 1 }}>
        <div style={{ background: 'gray' }}>Full</div>
        <Grid>
          <Column sm={2} md={3} lg={4}>
            <div style={{ background: 'gray' }}>Half</div>
          </Column>
          <Column sm={2} md={3} lg={4}>
            <div style={{ background: 'gray' }}>Half</div>
          </Column>
          <Column sm={2} md={3} lg={4}>
            <div style={{ background: 'gray' }}>Half</div>
          </Column>
          <Column sm={true} md={true} lg={true}>
            Auto col
          </Column>
        </Grid>
      </Column>
    </Grid>
  );
};

@janhassel janhassel requested a review from a team as a code owner October 5, 2021 15:12
@janhassel janhassel requested review from aledavila and dakahn October 5, 2021 15:12
@netlify
Copy link

netlify bot commented Oct 5, 2021

✔️ Deploy Preview for carbon-react-next ready!

🔨 Explore the source changes: f0798d9

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-react-next/deploys/616725b697e0590007f1c455

😎 Browse the preview: https://deploy-preview-9803--carbon-react-next.netlify.app

@tay1orjones tay1orjones self-requested a review October 5, 2021 15:52
@netlify
Copy link

netlify bot commented Oct 5, 2021

✔️ Deploy Preview for carbon-components-react ready!
Built without sensitive environment variables

🔨 Explore the source changes: f0798d9

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-components-react/deploys/616725b69572990007f25180

😎 Browse the preview: https://deploy-preview-9803--carbon-components-react.netlify.app

@netlify
Copy link

netlify bot commented Oct 5, 2021

✔️ Deploy Preview for carbon-elements ready!

🔨 Explore the source changes: f0798d9

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-elements/deploys/616725b55a3c7000096febbf

😎 Browse the preview: https://deploy-preview-9803--carbon-elements.netlify.app

Copy link
Member

@tay1orjones tay1orjones left a comment

Choose a reason for hiding this comment

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

I have one question on my review below, but both your example story and the existing offset story look great.

proper.offset.column.reflow.mov

I pushed one small update to this - I noticed the existing stories' styles weren't being applied due to the prefix change.

packages/grid/scss/modules/_css-grid.scss Show resolved Hide resolved
Copy link
Contributor

@dakahn dakahn left a comment

Choose a reason for hiding this comment

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

lgtm

@kodiakhq kodiakhq bot merged commit 9f558cf into carbon-design-system:main Oct 13, 2021
@janhassel janhassel deleted the subgrid-offset branch October 15, 2021 17:29
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.

4 participants