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

remove bourbon #1883

Merged
merged 12 commits into from
Dec 12, 2017
Merged

remove bourbon #1883

merged 12 commits into from
Dec 12, 2017

Conversation

giladgray
Copy link
Contributor

@giladgray giladgray commented Dec 11, 2017

Fixes #875

Changes proposed in this pull request:

🔥 remove bourbon dependency core (and devDep from all other packages)

we used three things from bourbon:

  1. @mixin font-face() for icons: added our own font-face mixin based on original Bourbon source
  2. @mixin position() was used heavily as shorthand for position properties. I added @mixin position-all(absolute, 0) for the simple set-all-sides-to-the-same-value case, and expanded all other usages to the individual CSS properties.
  3. @mixin prefixer() was used for appearance prop in a few places; replaced with actual CSS properties.

@blueprint-bot
Copy link

oops missed one

Preview: documentation

@blueprint-bot
Copy link

strip-unit()

Preview: documentation

Copy link
Contributor

@cmslewis cmslewis left a comment

Choose a reason for hiding this comment

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

Cool cool. How does this affect the bundle size?

// Removes the unit from a Sass numeric value (if present).
// strip-unit(12px) => 12
@function strip-unit($number) {
@if type-of($number) == "number" and not unitless($number) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just verifying: are these built-in SCSS helpers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

indeed they are

@@ -1,7 +1,7 @@
// Copyright 2015 Palantir Technologies, Inc. All rights reserved.
// Licensed under the terms of the LICENSE file distributed with this project.

@import "~bourbon/app/assets/stylesheets/bourbon";

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably don't need two newlines in a row here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah snap one slipped through!

@cmslewis
Copy link
Contributor

p.s. @giladgray you have a merge conflict with docs/package.json.

@giladgray
Copy link
Contributor Author

giladgray commented Dec 12, 2017

@cmslewis this won't affect bundle size at all because Bourbon only exists at compile time: it's a bunch of mixins and functions, used to generate CSS.

@blueprint-bot
Copy link

remove extra line

Preview: documentation

border-radius: $pt-border-radius;
height: $pt-button-height;
padding: 0 ($input-padding-horizontal * 3) 0 $input-padding-horizontal;
-moz-appearance: none;
Copy link
Contributor

Choose a reason for hiding this comment

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

ooc, none of our tooling does automatic prefixing by default?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oof actually no, we don't run autoprefixer on the generated CSS file. one of the things we lost in the (gulp) fire, along with comment stripping.

// strip-unit(12px) => 12
@function strip-unit($number) {
@if type-of($number) == "number" and not unitless($number) {
@return $number / ($number * 0 + 1);
Copy link
Contributor

Choose a reason for hiding this comment

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

$number / 1?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

12px / 1 == 12px. but 12px / (0px + 1) == 12
unit math, my friend!

@@ -119,8 +119,8 @@ label.docs-color-scheme-label { margin: ($pt-grid-size * 2) 0; }
}

&::before {
@include position(absolute, -2px);
border-radius: 4px;
@include position-all(absolute, -$palette-border-radius / 2);
Copy link
Contributor

Choose a reason for hiding this comment

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

👌

@blueprint-bot
Copy link

cite source

Preview: documentation

// https://github.com/thoughtbot/bourbon/blob/master/core/bourbon/library/_font-face.scss
// https://github.com/thoughtbot/bourbon/blob/master/core/bourbon/utilities/_font-source-declaration.scss

/// Generates an `@font-face` declaration. You can choose the specific file
Copy link
Contributor

Choose a reason for hiding this comment

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

why the triple slashes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I dunno, copy-pasted from bourbon source. I think they use it for doc comments? I'll revert to doubles

@blueprint-bot
Copy link

double slashes

Preview: documentation | table

@adidahiya adidahiya merged commit 77eac90 into master Dec 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants