Skip to content
This repository has been archived by the owner on Apr 13, 2022. It is now read-only.

[Color system] Add color factory and build modern tokens #105

Merged
merged 5 commits into from
Feb 20, 2020

Conversation

danrosenthal
Copy link
Contributor

@danrosenthal danrosenthal commented Feb 20, 2020

Merging the next branch into master, which includes several changes after the v2.7.0 release, as well as the latest changes incorporating the color factory into tokens, and using TypeScript in the build for modern tokens.

Release candidate on UNPKG: https://unpkg.com/browse/@shopify/polaris-tokens@2.8.0-rc.0/dist-modern/

This will all be released as v2.8.0, since these changes are not breaking.

@danrosenthal
Copy link
Contributor Author

danrosenthal commented Feb 20, 2020

@kaelig, should we be merging f471512 and 68772b3 into master along with this, or should those remain unmerged in the next branch?

Looks like those should not be merged to master. I will drop those commits from this PR.

@kaelig kaelig temporarily deployed to polaris-toke-merge-next-sv5n9p February 20, 2020 15:34 Inactive
@danrosenthal danrosenthal changed the title [Color system] Merge next to master [Color system] Add color factory and build modern tokens Feb 20, 2020
@kaelig kaelig temporarily deployed to polaris-toke-merge-next-sv5n9p February 20, 2020 15:37 Inactive
@kaelig kaelig temporarily deployed to polaris-toke-merge-next-sv5n9p February 20, 2020 15:39 Inactive
@kaelig kaelig temporarily deployed to polaris-toke-merge-next-sv5n9p February 20, 2020 15:40 Inactive
Co-authored-by: Dan Rosenthal <dan.rosenthal@shopify.com>
Co-authored-by: Ben Scott <227292+BPScott@users.noreply.github.com>
Co-authored-by: Kyle Durand <kyle.durand@shopify.com>
Co-authored-by: Sara Hill <sara.hill@shopify.com>
@kaelig kaelig temporarily deployed to polaris-toke-merge-next-sv5n9p February 20, 2020 15:42 Inactive
Copy link
Member

@BPScott BPScott left a comment

Choose a reason for hiding this comment

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

RC package contents look good to me from looking at unpkg, though I haven't tried using it within polaris-react to confirm end-to-end

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@@ -121,7 +120,7 @@ thead th:first-child {
}

code {
font-family: Monaco, Consolas, 'Lucida Console', monospace;
font-family: Monaco, Consolas, "Lucida Console", monospace;
Copy link
Contributor

@kaelig kaelig Feb 20, 2020

Choose a reason for hiding this comment

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

I thought the convention was single quotes – if so, can you check if there is a Sass or Prettier configuration issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@BPScott updated the prettier config to not format these files, so not an issue.

Ben, what was your rationale there?

Copy link
Member

@BPScott BPScott Feb 20, 2020

Choose a reason for hiding this comment

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

This was deliberately changed - The docs folder is no longer formatted with prettier (but I do use sass's expanded style to maintain most of the readable formatting :)

I split the build into "legacy", "modern" and "docs" steps, this makes it easy to do just the modern build in shipit (to generate the dist-modern folder), and also change the docs build to something new (probably a proper gatsby app) in the near future. In order to retain the old behaviour I would have had to either:

  • have two steps "build-legacy:format" (to format the dist folder with prettier) and "docs:format" (to format the css in the docs folder with prettier) to that run after each of the build-legacy and docs steps
  • A single step that runs after both "build-legacy" and "docs" - which makes makes the of steps a bit more convoluted and less self-contained.

I figured seeing as this file is generated content, and is going away imminently it is easier to only create a build-legacy:format step that formats just the dist folder, and skip formatting the docs folder css with prettier, as sass already does a pretty solid job. Incidentally I've just realised by formatting this file after sass does its thing we risk breaking the css.map file that the sass build generates (as that maps locations in the not-prettiered css file, not the version that's been ran through prettier)

danrosenthal and others added 2 commits February 20, 2020 12:14
Co-Authored-By: Kaelig Deloumeau-Prigent <kaelig@users.noreply.github.com>
Co-Authored-By: Kaelig Deloumeau-Prigent <kaelig@users.noreply.github.com>
@kaelig kaelig temporarily deployed to polaris-toke-merge-next-sv5n9p February 20, 2020 17:14 Inactive
@kaelig kaelig temporarily deployed to polaris-toke-merge-next-sv5n9p February 20, 2020 17:14 Inactive
@kaelig kaelig temporarily deployed to polaris-toke-merge-next-sv5n9p February 20, 2020 17:50 Inactive
@danrosenthal danrosenthal merged commit f80ce5c into master Feb 20, 2020
@danrosenthal danrosenthal deleted the merge-next-to-master branch February 20, 2020 18:08
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants