Add marketing buttons to primer-marketing-buttons #337

gladwearefriends · 2017-09-06T22:19:59Z

Testing on github/github here https://github.com/github/github/pull/81105 with the marketing buttons styles removed too.
Deployed onto review lab: https://testing-marketing-buttons-primer.review-lab.github.com/home

To test on review-lab, you can slap these classes onto one of the buttons on the homepage to test:

.btn-orange
.btn-outline-green
.btn-transparent
.btn-large

This has also been tested in StoryBook.

This PR needs to be deployed after Primer 10 is released https://github.com/github/github/pull/81106

This isn't in primer yet! https://github.com/github/github/blob/master/app/assets/stylesheets/custom/site/buttons.scss

Not sure if its related but i remembered this because I was checking out the style guide and the outline styles didnt load
https://styleguide.github.com/primer/components/marketing-buttons/

First time contributing 😬 Let me know if I did that right or not please!

/cc @primer/ds-core

gladwearefriends · 2017-09-06T22:22:33Z

I'll open a PR to delete from github/github once I get the 👍

broccolini

Hi @gladwearefriends thanks for doing this! Adding these were actually on my ship list for Primer v10 so you've done some of the work for me 😄

There are a few more steps for including new modules in Primer though, if you can start with the feedback I've given here, I'll help you with the next steps when you're ready!

broccolini · 2017-09-06T22:31:29Z

modules/primer-marketing-buttons/README.md

@@ -0,0 +1,91 @@
+# Primer Marketing CSS Typography


Looks like you copy+pasted the README for marketing Typography. The Docs for marketing buttons are mostly done and live in github so you can copy that over, (long story but someone started creating a package but didn't actually add it to Primer lol), but not all the buttons you're adding here are in there, such as btn-transparent and btn-blurple—though I'd like to question the use of that button and/or a more sensible name before merging this in.

broccolini · 2017-09-06T22:31:53Z

modules/primer-marketing-buttons/README.md

@@ -0,0 +1,91 @@
+# Primer Marketing CSS Typography
+
+[![npm version](http://img.shields.io/npm/v/primer-marketing-type.svg)](https://www.npmjs.org/package/primer-marketing-type)


You'll need to update these urls to primer-marketing-buttons.

broccolini · 2017-09-06T22:32:32Z

modules/primer-marketing-buttons/README.md

+[![npm version](http://img.shields.io/npm/v/primer-marketing-type.svg)](https://www.npmjs.org/package/primer-marketing-type)
+[![Build Status](https://travis-ci.org/primer/primer-css.svg?branch=master)](https://travis-ci.org/primer/primer-css)
+
+> Flash messages, or alerts, inform users of successful or pending actions. Use them sparingly. Don’t show more than one at a time.


And this description should be for marketing buttons too... now going to check if this description is in marketing typography O_o

so it should be in marketing buttons? it was copied and pasted from marketing type. i just copy and pasted the message from primer-buttons instead.

broccolini · 2017-09-06T22:33:19Z

modules/primer-marketing-buttons/package.json

@@ -0,0 +1,42 @@
+{
+  "version": "1.3.0",


The version number should be 0.0.1 until we're ready to ship.

broccolini · 2017-09-06T22:33:44Z

modules/primer-marketing-buttons/package.json

+  "main": "build/index.js",
+  "primer": {
+    "category": "marketing",
+    "module_type": "utilities"


module_type should be components.

broccolini · 2017-09-06T22:34:09Z

modules/primer-marketing-buttons/package.json

+    "lib",
+    "build"
+  ],
+  "repository": "https://github.com/primer/primer-css/tree/master/modules/primer-marketing-type",


You'll need to update the url to primer-marketing-buttons.

shawnbot

Forgive me if this wasn't ready to go, but I noticed a bunch of stuff that was carried over from the marketing-type package.

shawnbot · 2017-09-06T22:30:04Z

modules/primer-marketing-buttons/README.md

@@ -0,0 +1,91 @@
+# Primer Marketing CSS Typography


"Typography" should be "Buttons" here, right?

shawnbot · 2017-09-06T22:30:53Z

modules/primer-marketing-buttons/README.md

@@ -0,0 +1,91 @@
+# Primer Marketing CSS Typography
+
+[![npm version](http://img.shields.io/npm/v/primer-marketing-type.svg)](https://www.npmjs.org/package/primer-marketing-type)


Similarly, you can probably/safely search for "marketing-type" and replace with "marketing-buttons".

shawnbot · 2017-09-06T22:31:09Z

modules/primer-marketing-buttons/README.md

+## Documentation
+
+<!-- %docs
+title: Typography


"Marketing Buttons"?

shawnbot · 2017-09-06T22:32:35Z

modules/primer-marketing-buttons/README.md

+status: New Release
+-->
+
+The typography for our marketing pages differs slightly from what is in Primer's core--it is responsive, on a slightly different scale, and headlines are in a different font (Roboto).


Everything between the  markers should be updated with buttons docs.

shawnbot · 2017-09-06T22:35:31Z

modules/primer-marketing-buttons/package.json

+  "main": "build/index.js",
+  "primer": {
+    "category": "marketing",
+    "module_type": "utilities"


I think this should be components; at least, that's how we have it in the style guide right now.

shawnbot · 2017-09-06T22:36:44Z

modules/primer-marketing-buttons/package.json

@@ -0,0 +1,42 @@
+{
+  "version": "1.3.0",


I think this should start at 0.0.1, but @broccolini can probably suggest a more appropriate version number since this package is probably more "mature".

When this is ready to ship we can bump to major, but I think we should do that in the v10 pr.

shawnbot · 2017-09-06T22:37:42Z

modules/primer-marketing/index.scss

@@ -16,6 +16,7 @@

 // marketing specific css modules
 @import "primer-marketing-type/index.scss";
+@import "primer-marketing-buttons/index.scss";


In order for this to work, we'll also need to add this to the dependencies list in modules/primer-marketing/package.json. You should use the same version as you provide in this module's package.json.

broccolini · 2017-09-06T23:12:26Z

modules/primer-marketing-buttons/lib/button.scss

+.btn-outline-green { @include btn-outline($green); }
+
+@mixin btn-transparent-active {
+  color: #333;


Forgot to add, can you replace the hex values with new color variables? We also have variables for line-height, border, and border-radius too (for use further down in btn-transparent.

@broccolini will do! im actually going to remove a few declarations underbtn-transparent on second thought a lot of these should be inherited from .btn already

broccolini · 2017-09-06T23:19:53Z

modules/primer-marketing-buttons/lib/button.scss

+.btn-transparent {
+  padding: 0.55em 1em;
+  font-size: inherit;
+  font-weight: 500; // to avoid faux-bolding


I'm curious about what "faux-bolding" means, we've updated all our font-weights to bold (600), or normal (400) in most other places. If we do need it, you can use the $font-weight-semibold variable instead.

i also just remembered a few of these styles from github/github (where i copied them over from) were declared specifically for an instance where .btn-transparent was used on the marketing homepage. ive removed the font-weight, font-size, padding, line-height, and border-radius as they should be inherited from .btn or the other btn size classes, e.g. .btn-large

gladwearefriends · 2017-09-07T00:01:39Z

i made updates!

simplified btn-transparent declarations to fall back on .btn styles. from a comment i posted for @broccolini

i also just remembered a few of these styles from github/github (where i copied them over from) were declared specifically for an instance where .btn-transparent was used on the marketing homepage. ive removed the font-weight, font-size, padding, line-height, and border-radius as they should be inherited from .btn or the other btn size classes, e.g. .btn-large

i noticed primer-marketing-support isnt needed as a dependency for primer-marketing-buttons and took that out.
i also moved .btn-large from the primer-buttons module into primer-marketing-buttons as well as it looks like large should only be used on marketing? that change kind of scares me if others are already using btn-large from primer-buttons.
updated primer-marketing-buttons readme to reflect these changes

let me know if im approaching all of these things correctly! appreciate the pointers 🙏

shawnbot · 2017-09-11T17:21:27Z

Hey @gladwearefriends, I can help you wrap this up this week.

shawnbot · 2017-09-22T20:41:24Z

FYI, I've changed the base branch to merge this into our v10 release branch. @broccolini, can you re-review this?

broccolini · 2017-09-22T21:12:13Z

modules/primer-css/package.json

@@ -42,6 +42,7 @@
    "primer-layout": "1.3.0",
    "primer-markdown": "3.6.0",
    "primer-marketing": "5.3.1",
+    "primer-marketing-buttons": "0.0.1",


testing testing

jonrohan

This looks great to me 💯

gladwearefriends · 2017-11-03T17:54:50Z

Testing on github/github here https://github.com/github/github/pull/81105

gladwearefriends · 2017-11-03T18:01:54Z

Related PR that needs to be deployed in github/github after Primer 10 is released https://github.com/github/github/pull/81106

This pr removes the styles that are now moved into the primer-marketing package under primer-marketing-buttons

broccolini

Thanks for working on this @gladwearefriends 💖

Requesting documentation changes and questions about usage.

broccolini · 2017-11-05T15:56:12Z

CHANGELOG.md

@@ -5,10 +5,12 @@
 - Importing `.input-group` into `primer-forms` module.
 - New module `primer-branch-name` "A nice, consistent way to display branch names."
 - New module `primer-dropdown` "A lightweight context menu for navigation and actions."
+- New module `primer-marketing-buttons`. Buttons for marketing websites at GitHub


We're going to get mad merge conflicts in the v10 branch if everyone updates the changelog (hadn't thought of this before), can you remove the changes here and just add a comment to your pr with "To be included in the Changelog" so we can copy the updates over.

broccolini · 2017-11-05T16:05:55Z

modules/primer-buttons/lib/button.scss

@@ -88,13 +88,6 @@
  line-height: 20px;
 }

-// Large button adds more padding around text. Use font-size utils to increase font-size.. e.g, <p class="text-gamma"><button class="btn btn-large btn-primary" type="button">Big green button</button></p>
-.btn-large {


This is used in some locations outside of /site/ directory - are you sure this is not needed for product? I think there are some instances of people using utilities to make buttons larger too. One location I can think of is the dashboard banner when you are a new user - easiest way to see this is to run the dev environment locally.

im moving btn-large back into primer core here! not sure why i moved it into marketing-buttons other than i thought it was only used on marketing pages 🤷🏻‍♀️

broccolini · 2017-11-05T18:43:31Z

modules/primer-css/package.json

@@ -41,6 +41,7 @@
    "primer-labels": "1.5.1",
    "primer-layout": "1.4.1",
    "primer-markdown": "3.7.1",
+    "primer-marketing-buttons": "0.0.1",


Note to change this to 1.0.0 after merging into v10 branch and run bump script. cc @jonrohan

broccolini · 2017-11-05T19:38:15Z

modules/primer-marketing-buttons/package.json

+    "test": "../../script/npm-run-all build lint test-docs"
+  },
+  "dependencies": {
+    "primer-support": "4.4.1"


These buttons are designed to be used with btn so they can't be used without installing primer-buttons. It's not a code dependency, but perhaps should be a peer dependency? What do you think @jonrohan?

I think peerDependency makes sense here. Be sure when you manually add it, that it's the current version of primer-buttons

I just had a flash back to trying to setup peer dependencies with Lerna from the original monorepo set up and it wasn't pretty. @gladwearefriends leave that to us, maybe it's all fine now since there's been lots of changes but I'd rather us deal with problems if they come up than you! We can do this before merging in tomorrow.

broccolini · 2017-11-05T19:48:10Z

modules/primer-marketing-buttons/lib/button.scss

@@ -0,0 +1,32 @@
+.btn-orange { @include btn-solid($white, lighten($orange-500, 7.5%), darken($orange-500, 7.5%)); }
+.btn-blurple { @include btn-solid($white, mix($blue-400, $purple-400, 35%), mix($blue-600, $purple-600, 35%)); }


I'd like to give this a legit class name (such as indigo), and I'd like to discuss why we have this exception regarding colors. I think we should very rarely use a color in our UI for a single use case that is not used in any other way on our site. Do we really need this extra color? Could it be a shade variation of existing colors, or use another existing button color, such as btn-blue? Or, do we need to add indigo to our color system so that it may be used in more places than just this one button? cc @sophshep

These are good questions @broccolini. It looks like theyre only used on the business pages here: https://github.com/business and on app/views/coupons/business_plus_only/show.html.erb (not sure where that view is). I'm for keeping this as a one off style in github/github. im not sure if its used anywhere else on other one off marketing pages outside of github/github and it does seem weird to go out of the primer color system. What do you think @sophshep ?

At this point im gonna leave it out of this PR. I can put it back in if we think we should do it.

broccolini · 2017-11-05T19:57:30Z

modules/primer-marketing-buttons/README.md

+
+## Color
+
+The `btn` has been extended with the marketing color palette:


Suggest changing to more descriptive sentence:

Marketing buttons are available in orange, purple, and ~~blurple~~ indego.

😬

broccolini · 2017-11-05T20:01:50Z

modules/primer-marketing-buttons/README.md

+<a class="btn btn-outline-purple" href="#url" role="button">Link button</a>
+<button class="btn btn-outline-green" type="button">Button button</button>
+<div class="bg-gray-dark p-4 mt-4">
+  <button class="btn btn-transparent" type="button">


btn-transparent shouldn't be grouped with the outline options, it should have is own description and be a separate example.

broccolini · 2017-11-05T20:36:21Z

modules/primer-marketing-buttons/README.md

+```
+
+## Additional sizing
+There are cases where you might want to increase the size of a button, for example when putting a main CTA inside of a jumbotron or major page callout. The `btn-large` class does the following to make a button more prominent:


I think the intro paragraph can be more concise, and I don't think we need to call out the description in a numbered list or get into exact values, suggest changing to something like the following:

Use btn-large to increase the padding and border radius of a button. This is useful for prominent calls to action, and is often used in combination with jumbtron styles.

Type scale utilities can be used to alter the font-size if needed. Padding is applied in em's so that it scales proportionally with the font-size.

Side note: As someone less familiar with marketing styles, I had no idea what a jumbotron might be. I looked it up and this looks like it could be called hero which I think is more meaningful to people. We try to stick with class names that are obvious, transparent, and human-readable—could we change this to something more meaningful in future? Related, minitron and supertron could just me small and large 🤷‍♀️. I get this is probably just a bit of humor, but it's un-helpful for people learning to use Primer. cc @sophshep

I've changed the language in this section of the docs to "This is useful for prominent calls to action in hero sections."

Related, this section of the docs pertaining to btn-large is also moved into primer-buttons' readme.md as btn-largehas been moved back to primer-buttons as well, where it originally was just without documentation.

broccolini · 2017-11-05T20:38:13Z

modules/primer-marketing-buttons/README.md

+
+```html
+
+<p class="f3">


Show how the default btn-large looks without a font size change, then show another example with a type scale utility.

broccolini · 2017-11-05T20:40:40Z

modules/primer-marketing-buttons/lib/button.scss

+  border-color: $white;
+}
+
+.btn-transparent {


This doesn't seem to be used anywhere in the github/github is it being used elsewhere for marketing sites?

It is sometimes used on the ads we have on the homepage below the hero on github.com. I personally have also used it here on the hero and circular callouts https://partner.github.com/

gladwearefriends · 2017-11-07T03:07:51Z

To be included in the Changelog (as @broccolini requested above at #337 (comment))

New module primer-marketing-buttons. Buttons for marketing websites at GitHub.

just realized the old marketing button documentation threw me off. only orange is being added.

…pendencies

broccolini requested changes Sep 6, 2017

View reviewed changes

shawnbot suggested changes Sep 6, 2017

View reviewed changes

broccolini added the v10 label Sep 6, 2017

broccolini mentioned this pull request Sep 6, 2017

Tracking issue for Primer v10 #335

Closed

28 tasks

broccolini reviewed Sep 6, 2017

View reviewed changes

gladwearefriends force-pushed the add-marketing-buttons branch from 33fab2a to 9599372 Compare September 13, 2017 22:42

shawnbot approved these changes Sep 15, 2017

View reviewed changes

shawnbot changed the base branch from dev to release-10.0.0 September 22, 2017 20:39

broccolini reviewed Sep 22, 2017

View reviewed changes

gladwearefriends force-pushed the add-marketing-buttons branch from ed7cb6a to 1d3a39f Compare November 2, 2017 22:43

jonrohan force-pushed the add-marketing-buttons branch from 1d3a39f to ed7cb6a Compare November 2, 2017 22:47

gladwearefriends and others added 13 commits November 2, 2017 15:55

add marketing buttons to primer-marketing-buttons

41f157b

add import to primer-marketing

a6db97c

update read me for buttons

2e039ad

actually update the marketing buttons readme for reals

bfeb14a

update package.json

ece1533

use variables and move .btn-large into primer-marketing-buttons

d16903a

edit dependencies, does not need primer-marketing-support

234e3b2

edit package.json

ca26cfc

update readme to pass test

1078145

Updating scripts

da1c1c4

Updating changelog

0383073

Using variable for fade

a1918c9

Updating version

69685d9

gladwearefriends force-pushed the add-marketing-buttons branch from ed7cb6a to 69685d9 Compare November 2, 2017 23:03

gladwearefriends added 2 commits November 2, 2017 16:13

add primer-marketing-buttons to primer-marketing package.json

f1681f3

add stories.js for storybook

22c7f75

jonrohan approved these changes Nov 3, 2017

View reviewed changes

broccolini requested changes Nov 5, 2017

View reviewed changes

gladwearefriends and others added 9 commits November 6, 2017 21:25

rm changelog update

fc30322

rm btn-blurple

1c81f3c

use border variables

cea6e67

move btn-large back into primer-buttons, update docs

60a8d27

move btn-large test into primer-buttons’ stories.js

171e0b6

only btn-orange is being added

43e12e8

just realized the old marketing button documentation threw me off. only orange is being added.

test wants an example of counter

7f9fc05

Setting up peer dependency for marketing buttons, fixing generator de…

73b5205

…pendencies

revert changelog

6c4d49d

jonrohan merged commit 324c230 into release-10.0.0 Nov 7, 2017

jonrohan deleted the add-marketing-buttons branch November 7, 2017 22:15

jonrohan added the Tag: Enhancement label Nov 10, 2017

github-actions bot mentioned this pull request Nov 28, 2022

Link Checker Report ronna/MERL-Center-public#20

Open

github-actions bot mentioned this pull request Dec 19, 2022

Link Checker Report ronna/MERL-Center-public#48

Open

github-actions bot mentioned this pull request Jan 10, 2023

Link Checker Report ronna/MERL-Center-public#92

Open

		@@ -0,0 +1,91 @@
		# Primer Marketing CSS Typography

		[![npm version](http://img.shields.io/npm/v/primer-marketing-type.svg)](https://www.npmjs.org/package/primer-marketing-type)

		@@ -0,0 +1,32 @@
		.btn-orange { @include btn-solid($white, lighten($orange-500, 7.5%), darken($orange-500, 7.5%)); }
		.btn-blurple { @include btn-solid($white, mix($blue-400, $purple-400, 35%), mix($blue-600, $purple-600, 35%)); }


		## Color

		The `btn` has been extended with the marketing color palette:


		```html

		<p class="f3">

Add marketing buttons to primer-marketing-buttons #337

Add marketing buttons to primer-marketing-buttons #337

Conversation

gladwearefriends commented Sep 6, 2017 • edited

gladwearefriends commented Sep 6, 2017

broccolini left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shawnbot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gladwearefriends Sep 6, 2017 • edited

Choose a reason for hiding this comment

gladwearefriends commented Sep 7, 2017 • edited

shawnbot commented Sep 11, 2017

shawnbot commented Sep 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonrohan left a comment

Choose a reason for hiding this comment

gladwearefriends commented Nov 3, 2017 • edited

gladwearefriends commented Nov 3, 2017

broccolini left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gladwearefriends commented Nov 7, 2017 • edited

gladwearefriends commented Sep 6, 2017 •

edited

gladwearefriends Sep 6, 2017 •

edited

gladwearefriends commented Sep 7, 2017 •

edited

gladwearefriends commented Nov 3, 2017 •

edited

gladwearefriends commented Nov 7, 2017 •

edited