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

Move responsive position utilities from marketing to core #459

Merged
merged 6 commits into from Jan 23, 2019

Conversation

sophshep
Copy link

This PR moves the responsive position styles (and their docs) from primer-marketing into primer-core.

This first came up as a need in this GitHub PR, but I suspect that as we're doing more and more responsive work in the product that these will be valuable in primer-core in the future.

/cc @primer/ds-core @jonrohan

@mdo
Copy link
Contributor

mdo commented Mar 14, 2018

This might not be the best time to ask this, but I'd love to see these utilities broken apart more. While working on the Blog migration awhile back, many of these weren't useful for a basic layout and this one file mixes utilities that affect inline-ish elements (vertical-align), text direction, and position. None of these aside from position screams "layout" to me, but admittedly my experience is limited to Primer 9.x several months ago. Dunno what else has changed since then or if our approach in the Blog even changed since then.

@shawnbot
Copy link
Contributor

I feel good about this, but I'm on the fence about whether this is a minor or major change. On one hand, moving styles feels major to me because it "breaks" people's assumptions about what CSS will be output when the @import certain SCSS files. On the other hand, is there any possible use cae for primer-marketing without primer-core?

@sophshep
Copy link
Author

is there any possible use case for primer-marketing without primer-core?

I can't think of any projects where I've used primer-marketing without primer-core.

@shawnbot
Copy link
Contributor

Well, I went searching, and here's what I found:

  • Most search hits for dependencies + primer-marketing turned up what appear to be forks of government.github.com that all depend on the same versions of primer-core and primer-marketing, so yay!
  • Except! githubuniverse.com appears to depend on primer-marketing but not primer-core. See: package.json
  • SO MANY PEOPLE HAVE node_modules COMMITTED TO GIT! 😱

@jonrohan jonrohan added this to TODO: v11.0.0 in 📦 Primer CSS release tracking Jun 25, 2018
@jonrohan jonrohan changed the base branch from dev to master June 25, 2018 20:18
@shawnbot shawnbot changed the base branch from master to release-11.0.0 October 5, 2018 23:52
@shawnbot
Copy link
Contributor

shawnbot commented Oct 5, 2018

@sophshep I changed the base branch of this so that it'll merge (eventually) into #498, but that branch is itself way out of date with master. Let's take a look at getting this wrapped up next week.

@jonrohan jonrohan mentioned this pull request Oct 17, 2018
22 tasks
@shawnbot
Copy link
Contributor

@sophshep how would you feel about merging #545 into this and using that pattern here, too?

relative,
absolute,
fixed
) !default;
Copy link
Contributor

Choose a reason for hiding this comment

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

Heads up @mdo: I added this variable so that responsive position utilities can be customized. For instance, if you only use relative or absolute positioning, you can do something like:

$responsive-positions: (relative, absolute);
@import "primer-core/index.scss";

And instead of 20 position utilities, you'd get 10.

Copy link
Contributor

@shawnbot shawnbot left a comment

Choose a reason for hiding this comment

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

@sophshep I've made some changes and am approving for inclusion in v11: #498. Thank you for this! 🎉

@@ -30,6 +30,7 @@ const exceptions = {
'/packages/primer-product': removed,
'/principles/HTML': moved('/principles/html'),
'/principles/SCSS': moved('/principles/scss'),
'/utilities/marketing-layout': moved('/utilities/layout'),
Copy link
Contributor

Choose a reason for hiding this comment

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

This is actually pretty cool: removing primer-marketing-utilities/docs/layout.md caused the /utilities/layout page to disappear, which triggered a failing test. I had to mark that URL as having moved in order to get it passing again.

@shawnbot shawnbot merged commit 7e8b2da into release-11.0.0 Jan 23, 2019
📦 Primer CSS release tracking automation moved this from v11.0.0 Release to 💜 Done Jan 23, 2019
@shawnbot shawnbot deleted the responsive-layout-in-core branch January 23, 2019 04:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💓collab a vibrant hub of collaboration Tag: Breaking Change
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants