-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Update marketing button styles #1352
Conversation
🦋 Changeset detectedLatest commit: 2cc4dbb The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
src/marketing/buttons/button.scss
Outdated
} | ||
.btn-sm-mktg { | ||
// stylelint-disable-next-line primer/spacing | ||
padding: rem(10px) rem($spacer-4) rem(12px); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
padding: rem(10px) rem($spacer-4) rem(13px);
I bumped up the bottom padding here slightly for when it uses Alliance so the padding is optically correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Primer CSS uses the "system" font, so might wanna have the same top/bottom padding so it doesn't look mis-aligned? And maybe also a less bold font-weight
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tweaked the small ones with more spacing below, and also:
- Reduced spacing on the sides, to $spacer-3, for better balance
- Added one pixel of spacing to the
lg
size modifier
Since these are primarily for use with Alliance, I think it's more important that they work with that font for now. In the cases when they are not used with Alliance (I'm not sure if they should be at all), the padding can be overridden. However, I think we should consider this a bug with how Alliance is set up, and we should investigate if we can export Alliance with different settings to prevent us from "hacking" the vertical alignment in this way.
Preview:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
However, I think we should consider this a bug with how Alliance is set up, and we should investigate if we can export Alliance with different settings to prevent us from "hacking" the vertical alignment in this way.
Yeah, I guess when we update the Marketing Typorgraphy there is no way around not to include Alliance. Is Alliance an open source font or is a license needed to use it? Looks like the later. So maybe we could include it in the marketing font stack and then document that all the marketing components are optimized for Alliance. But maybe out of scope for this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One question before going into more details: Can/should these marketing buttons be used on pages that support color modes?
Maybe fine except for the btn-mktg btn-outline-mktg
that could use some more contrast in dark mode? Or the btn-transparent
is the dark mode version of btn-outline-mktg
?
In case they are only meant for light mode, maybe it needs to be documented somewhere. And we need a way to not show the color modes toggle in the docs?
Asked in Slack.
Looked into it a bit more and how I understood it: We have 3 kinds of marketing buttons.
This PR tries to replace the "Primer CSS Marketing" buttons with "Fluid" buttons.
Here some random pages that would probably be affected by this change:
Is that ok? Only tested it with some user-style hack, so might not be super accurate. |
Not just the buttons, we're merging any overlap between the two. Tracking issue is here https://github.com/github/site-design/issues/1190. It's part of our initiative to detangle our CSS tooling for marketing sites. The goal is to remove the
Yes 🙏 by further updating Primer Marketing
I think these mostly look OK! I've created a PR here https://github.com/github/github/pull/177537 to complete this transition with some more polish, to make sure that this PR in this repo doesn't produce any visual changes when pulled into .com. |
@simurai I've updated these to support color modes, and moved all variables to |
….com/primer/css into tobiasahlin/update-marketing-buttons
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I think this looks good. 👍
Eventually we'll have to figure what to do with the Alliance font, see #1352 (review), but we can follow up with that on another PR.
This PR aligns the marketing button styles to those used on GitHub.com, adds a purple variant (
.btn-purple-mktg
), a.btn-sm-mktg
modifier for small buttons, as well as updates the.btn-large-mktg
utility to use the more common-lg
modifier:.btn-lg-mktg
.This PR is dependent on
primitives
changes in primer/primitives#76.Preview of variants:
Preview of sizes:
Migration guide
.btn-large-mktg
with.btn-lg-mktg
/cc @primer/ds-core