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

Add more .border-white-fade utilities #713

Merged
merged 8 commits into from
Mar 13, 2019
Merged

Conversation

simurai
Copy link
Contributor

@simurai simurai commented Mar 8, 2019

This PR adds a few new .border-white-fade utility classes to be used on dark backgrounds.

  • Moves .border-white-fade from "marketing" to "core".
  • Adds multiple .border-white-fade-xx utilities to "marketing".

✨ New in core

screen shot 2019-03-08 at 4 58 40 pm

image

/cc @primer/ds-core

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.

Ack. @simurai it looks like there's already a border-white-fade in marketing utilities:

.border-white-fade {
border-color: $white-fade-15 !important;
}

...which is actually not a blocker for this PR! I like that we're adding a color variable and that the declaration (syntax-wise) matches the other border utilities. So this is a good change on the whole.

I think we just need to move some things around so that it's part of the utilities bundle rather than marketing/utilities. As for whether or not this would be a breaking change — in the sense that we shouldn't break sites that import marketing or marketing/utilities bundles, or even @primer/css/marketing/utilities/borders.scss directly — we have some options:

  1. We can move this (improved) .border-white-fade declaration into src/marketing/utilities/borders.scss, and move the import for it from src/marketing/utilities/index.scss to src/utilities/index.scss.

  2. We can leave the files in this PR as-is, remove the import from src/marketing/utilities/index.scss, and empty out src/marketing/utilities/borders.scss so that importing it has no effect. In this case I think we'd want to queue up removing it entirely for the next major release.

Any thoughts or concerns, @jonrohan, @sophshep @gladwearefriends?

@simurai
Copy link
Contributor Author

simurai commented Mar 12, 2019

Option 1 feels a bit weird, importing from src/marketing/utilities/borders.scss into src/utilities/index.scss, so I pushed option 2.

But maybe we can also reconsider option 3? Have a border color scale like @jonrohan initially suggested:

.border-black-fade-15 { border-color: rgba($black, 0.15) !important; }
.border-black-fade-30 { border-color: rgba($black, 0.3) !important; }
.border-black-fade-50 { border-color: rgba($black, 0.5) !important; }
.border-black-fade-70 { border-color: rgba($black, 0.7) !important; }
.border-black-fade-85 { border-color: rgba($black, 0.85) !important; }

.border-white-fade-15 { border-color: rgba($white, 0.15) !important; }
.border-white-fade-30 { border-color: rgba($white, 0.3) !important; }
.border-white-fade-50 { border-color: rgba($white, 0.5) !important; }
.border-white-fade-70 { border-color: rgba($white, 0.7) !important; }
.border-white-fade-85 { border-color: rgba($white, 0.85) !important; }

For example the border under "Teams" on the https://github.com/pricing page gets overridden two times. Both with different opacity. Which probably means there is a need for more border color options.

Screen Shot 2019-03-12 at 11 39 37 AM

Just not sure where to put them, core or only marketing? Maybe core? I only can think of the dark header, but there might be more places in the future.

@simurai simurai requested a review from shawnbot March 12, 2019 02:54
@simurai
Copy link
Contributor Author

simurai commented Mar 12, 2019

Just not sure where to put them, core or only marketing?

Ok, after some debating this PR now:

  • Moves .border-white-fade from "marketing" to "core".
  • Adds multiple .border-white-fade-xx utilities to "marketing".

This should cover the immediate needs and we can always more more to core at a later point.

@simurai simurai changed the title Add .border-white-fade utility Add more .border-white-fade utilities Mar 12, 2019
@simurai simurai mentioned this pull request Mar 12, 2019
10 tasks
@simurai simurai changed the base branch from master to release-12.2.0 March 12, 2019 12:59
@shawnbot
Copy link
Contributor

Okay @simurai, I took @jonrohan's request and moved the new classes to core as well. Let's see if we can get this wrapped up during our sync tonight/today!

From marketing to core
So that they work when using `border-bottom` etc.
For now to keep things consistent. We can support .border-bottom etc. at a later point.
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