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: Columns/Column styles to not rely on Stack styles #508

Merged
merged 2 commits into from
Apr 28, 2021

Conversation

gnapse
Copy link
Contributor

@gnapse gnapse commented Apr 26, 2021

Closes #...

Short description

For simplicity (and lazyness) the Columns component reused the Stack styles to make sure column cells also had the vertical spacing applied for when they are collapsed into a stack in narrower viewports (i.e. its feature of being responsive).

Since we changed the Stack styles recently, this broke the Columns styling. This PR brings back the styles but not they are part of the core Columns css and not a cross-component reference anymore (which admittedly was always a bad idea).

PR Checklist

  • Added tests for bugs / new features
  • Updated docs (storybooks, readme)
  • Executed npm run validate and made sure no errors / warnings were shown
  • Described changes in CHANGELOG.md
  • Bumped version in package.json and package-lock.json (npm --no-git-tag-version version <major|minor|patch>) ref
  • Updated all static build artifacts (npm run build-all)

Versioning

v9.2.0-beta.8

@gnapse gnapse requested a review from a team April 26, 2021 13:17
@gnapse gnapse self-assigned this Apr 26, 2021
@gnapse gnapse requested review from frankieyan and removed request for a team April 26, 2021 13:17
@gnapse
Copy link
Contributor Author

gnapse commented Apr 26, 2021

Github's gotta be kidding me. I assigned the product group so that I could get a random assignment (because I want to involve in these reviews more people than just Frankie and me), and it assignes Frankie anyway.

I was still going to assign Frankie manually after the fact, but it's funny how this "random" assignment often behaves.

Copy link
Member

@frankieyan frankieyan left a comment

Choose a reason for hiding this comment

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

Maybe it's a good opportunity to move away from the negative margins now that we're decoupling from the Stack component's styles, as it does make it a bit confusing to debug when looking at the devtools. Perhaps we can leverage the collapseBelow prop to apply the responsive classes we need here? Something like:

.collapse-below-tablet.space-xsmall .column:not(:first-child) {
	margin-top: var(--reactist-spacing-xsmall);
}

// The rest of the space rules...

@media (min-width: 768px /* --reactist-breakpoint-tablet */) {
    .collapse-below-desktop.space-xsmall .column:not(:first-child) {
	    margin-top: var(--reactist-spacing-xsmall);
    }

    // The rest of the space rules...
}

@media (min-width: 992px /* --reactist-breakpoint-desktop */) {
	.column:not(:first-child) {
	    margin-top: 0 !important;
    }
}

Perhaps combining the min-width and the max-width queries as well we can omit that desktop override.

@gnapse
Copy link
Contributor Author

gnapse commented Apr 27, 2021

I agree there's room for improvement, but unless there's an actual bug with the current approach, I'd like to move on with this PR for now. It is blocking the use of Columns right now in Twist, and I'd rather not take the detour now.

@gnapse gnapse merged commit aabbaf2 into beta Apr 28, 2021
@gnapse gnapse deleted the ernesto/design-system-fixes branch April 28, 2021 18:56
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.

2 participants