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

DP-18263: Creates new npm package for assets. #1032

Closed
wants to merge 15 commits into from

Conversation

smurrayatwork
Copy link
Contributor

@smurrayatwork smurrayatwork commented Apr 28, 2020

Any PRs being created needs a changelog.txt file before being merged into dev. See: Change Log Instructions

Description

This changes the assets directory to support being added as the massds scoped NPM package @massds/assets.

Notable Changes:

  • Node Sass has been replaced with Sass.
  • Mixins used from Bourbon Neat have been placed under 00-base/mixins/bourbon-neat and the package is no longer included / required.
  • The package no longer provides pre-compiled css files and instead requires the user to compile the included scss files. An example of how to do that has been provided in the README file.
  • The user is now required to include (at least once) the box-sizing mixin at 00-base/mixins/box-sizing. This file isn't really a mixin at all, but almost all patterns expect and require the rules from that file to exist somewhere in the stylesheet before they're used. Having each pattern include it causes that same box-sizing rule to be repeated every single time you include additional patterns.
  • All provided variables (including colors) have been changed to use ! default. Now users have the ability to override them as needed.

Next Steps:
Once this is approved, this should NOT be merged. We should publish the package to NPM first and ensure that packages that use our assets switch over to that first.

Related Issue / Ticket

Steps to Test

  1. You can use npm pack in the assets directory to create an example of what will be downloaded when a user installs the published package with npm install. That file can then be installed by running npm install path/to/file.tgz.

Screenshots

Additional Notes:

Anything else to add?

Impacted Areas in Application

@todo

  • Publish NPM package under the @massds/assets scope/name.
  • Ensure any other packages that use our assets can switch to the published version.
  • Create a separate repo for @massds/assets in Github.

Today I learned...

Some reccomended fonts you can use can be found under `00-base/fonts`.
## Example Compilation of Styles

The following example uses gulp to compile all .scss files under src (with exceptions to those .scss files under `00-base/`). The script will place the compiled files under `dist/` within the same directories they originated.
Copy link
Contributor

Choose a reason for hiding this comment

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

I threw this example into gulpfile.js and it's throwing error of variable undefined

Message:
    src/scss/00-base/mixins/_ma-button.scss
Error: Undefined variable.
   ╷```

Copy link
Contributor

Choose a reason for hiding this comment

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

let's follow in documentation improvement


## Setup and Usage (fonts/images)
Copy link
Contributor

Choose a reason for hiding this comment

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

So I think there are three use cases for this package:

  1. fonts images and colors variables
  2. SCSS
  3. compiled CSS (create an index.scss and compile that file using example gulp setup)

I think you have most of the things documented. Can we organize the documentation to support these use cases?

Copy link
Contributor

Choose a reason for hiding this comment

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

follow up in documentation improvement

assets/gulpfile.js Outdated Show resolved Hide resolved
assets/gulpfile.js Outdated Show resolved Hide resolved

@forward "color-tokens";

@forward "bourbon" show tint, clearfix;
Copy link
Contributor

Choose a reason for hiding this comment

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

this means we are only using tint and clearfix from bourbon?

return stream

.pipe(sass({
includePaths: [
Copy link
Contributor

Choose a reason for hiding this comment

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

In this example, maybe we can use assets/index.js that you showed me?

Also, I don't think index.js is in the dist/folder, only fonts images and scss are being moved.

@clairesunstudio
Copy link
Contributor

We also talked about in slack maybe removing the src/ and dist/ folder just to keep the assets directly at the package root. Just want to make sure the hot reload in storybook and patternlab still work for local development

"description": "Mayflower design tokens — it includes color token SCSS variables, fonts, icons and other imagery",
"main": "dist/scss/index",
"style": "dist/scss/index",
"name": "@massds/assets",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"name": "@massds/assets",
"name": "@massds/mayflower-assets",

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you also fix the steps in circleci config? I think just change mayflower-assets to mayflower-tokens and remove the build step?

…ng elements and fonts-styles files to 00-base.
// Let all your scss files have access to colors without having to remember that.
@forward "00-base/colors";
// Grab the fonts from this package for inclusion:
@forward "00-base/fonts";
Copy link
Contributor

Choose a reason for hiding this comment

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

fonts and other scss that refenrece $assets-path will need to config the var

@use "~@massds/assets/dist/scss/00-base/_fonts.scss" as * with (
  $assets-path: "~@massds/assets/dist"
);

Copy link
Contributor

Choose a reason for hiding this comment

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

follow up in documentation improvement

@clairesunstudio clairesunstudio changed the base branch from develop to mayflower-v10 April 30, 2020 14:50
@smurrayatwork
Copy link
Contributor Author

See #1037

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