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: allow nesting sized containers #593

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

joeholdcroft
Copy link

Description

Currently, if you try to nest a Container component with a smaller size than a Container component that wraps it at any level, the size does not correctly take effect. It always uses the largest size of any wrapping Container component due to CSS specificity.

This PR resolves this by applying the CSS for the sizing specifically to .rt-ContainerInner that are a direct descendent of the .rt-Container.

Testing steps

Use the following JSX and check the correct widths are applied in browser.

<Container size="4">
  <Text>This should be size 4</Text>
  <Container size="1">
    <Text>This should be size 1</Text>
  </Container>
</Container>

Alternatively, review the new Container samples in the "sink" demo example page.

Related issues / PRs

N/A

before this commit, it was not always possible to correctly nest container components with `size` set - for example, having a smaller container inside of a larger container. in these cases, css precedence meant the smaller size would never be applied.
Copy link

vercel bot commented Sep 23, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
themes-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 23, 2024 0:50am

@chaance
Copy link
Member

chaance commented Oct 4, 2024

Hmm, not entirely sure about the solution here. Having additional wrapping components will produce the same problematic results, no?

<Container size="4">
  <Text>This should be size 4</Text>
  <Box>
    <Container size="1">
      <Text>This should be size 1</Text>
    </Container>
  </Box>
</Container>

I'll play around with this a bit and see what we can do.

@joeholdcroft
Copy link
Author

Hmm, not entirely sure about the solution here. Having additional wrapping components will produce the same problematic results, no?

I think that works fine. The CSS I changed is for .ContainerInner which is directly inside of .Container, and the Container component renders both, I believe.

@chaance
Copy link
Member

chaance commented Oct 4, 2024

Ah yeah I think I missed that. I'll test locally and follow up as needed 👍

@keithburgie
Copy link

@chaance, I came across two open issues about this (#606 and #591) and was excited to submit exactly the same fix. @joeholdcroft's PR correctly solves the problem!

@joeholdcroft
Copy link
Author

@chaance anything I can do to help get this one through? 🙏

@chaance
Copy link
Member

chaance commented Dec 19, 2024

Still on my radar, just backed up with other priorities and need to spend a bit of time testing new features. I know it seems simple enough but unintended regressions w/ CSS specificity changes are not uncommon.

Long backlog, thanks for your patience!

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