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

Rename and move color mode variables out of base package #1462

Merged
merged 6 commits into from
Jun 16, 2021

Conversation

jonrohan
Copy link
Member

@jonrohan jonrohan commented Jun 14, 2021

Currently all of our color modes variables are included in the base/index.scss package. This pr moves the modes.scss files to the support/variables folder and imports them directly into primer/css/index. I'm not importing them into support/index because that package is meant to compile to nothing since it's being used in a lot of packages.

This is solving a few things:

  • Including the color modes in base, requires that we download all of primer/css bundle in github/github before we can start using color variables in custom stylesheets. Hoping to solve this issue https://github.com/github/github/issues/166084
  • Marketing and product packages that need color mode variables have to include all of the primer/core package

@changeset-bot
Copy link

changeset-bot bot commented Jun 14, 2021

🦋 Changeset detected

Latest commit: fa32602

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@primer/css Major

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Contributor

@simurai simurai left a comment

Choose a reason for hiding this comment

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

👍

Type: Minor

Should this be changed to a major release?

I guess minor is ok for sites/apps that load the /index.scss. But it would break when core/index.scss is loaded stand-alone because the color variables would not be available anymore. E.g. helphub.

@jonrohan
Copy link
Member Author

Should this be changed to a major release?

Yeah I agree, it was something I wasn't sure about.

@jonrohan jonrohan changed the base branch from main to next_major June 15, 2021 01:58
@jonrohan jonrohan changed the base branch from next_major to main June 15, 2021 02:00
@jonrohan jonrohan changed the base branch from main to next_major June 15, 2021 02:00
@jonrohan jonrohan enabled auto-merge (squash) June 15, 2021 02:15
@jonrohan jonrohan disabled auto-merge June 16, 2021 18:31
@jonrohan jonrohan merged commit 32040ff into next_major Jun 16, 2021
@jonrohan jonrohan deleted the move_color_modes branch June 16, 2021 18:31
jonrohan added a commit that referenced this pull request Sep 17, 2021
* Adding pre mode

* Updating the workflows

* Update README.md

* Create proud-rules-wonder.md

* Rename and move color mode variables out of base package (#1462)

* Rename and move color mode variables out of base package

* Create mighty-goats-teach.md

* Add test to make sure support is never more than 0

* Update mighty-goats-teach.md

* Update mighty-goats-teach.md

* Remove test

* Delete patch

* Moving color modes to their own bundle (#1465)

* Moving color modes to their own bundle

* Create new-beers-peel.md

* Removing the rem() mixin and usages. Placing the computed values in place. (#1541)

* Removing the rem() mixin and usages. Placing the computed values in place.

* Create flat-shirts-lay.md

* Fixing stylelint issues

* Fix more stylelint issue

* Remove break-word from utilities (#1566)

* Remove break-word from utilities

* Create twelve-fireants-shave.md

* Adding dark high contrast

* Fixing mising color modes selectors

* Remove pre.json if it exists

* Fixing lint error

* Removing pre mode

Co-authored-by: simurai <[email protected]>
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.

2 participants