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

Updating to USWDS 2.0.1 #421

Merged
merged 16 commits into from
Apr 22, 2019
Merged

Updating to USWDS 2.0.1 #421

merged 16 commits into from
Apr 22, 2019

Conversation

jaredcunha-usds
Copy link
Contributor

@jaredcunha-usds jaredcunha-usds commented Mar 3, 2019

Big changes between alpha and beta, so this update will allow teams to build more closely to the documentation in USWDS 2.0 when it's ready for release.

I still need to write tasks for preserving theme customizations (they are overwritten by USWDS gulp task)

Two outstanding items I'd like to cover:

1) how to handle deprecation in jekyll

DEPRECATION WARNING on line 327 of /Users/jaredcunha/work/website/assets/uswds-sass/core/_variables.scss:
#{} interpolation near operators will be simplified in a future version of Sass.
To preserve the current behavior, use quotes:

  unquote("#{$font-stack-alt}#{$if-important}")

2) Seeing how USWDS handles .5 system token issue in a future build
uswds/uswds-sandbox#18

What about this?
Do I just let gulp build the CSS instead of Jekyll? That means building CSS locally and having the compiled application.css under version control, but it is that worth the tradeoff? If that is the case, we can get rid of the assets/uswds-sass folder entirely, importing the files from node_modules instead. I'm kinda leaning towards this, and it seems the USWDS team prefers letting gulp build the CSS for now.

@dmethvin-gov
Copy link
Contributor

On the Jekyll SASS build problem: I can fix it by adding a line to _variables.css in $number-to-value that says '.5': '05', but of course that's inside USWDS files. I tried adding the needed entry to $number-to-value right after the include of _variables.css in application.scss but that didn't work, I think because the function that needs it (unit()) is called later in that file.

I can make a PR to USWDS to see if they'll accept it, not sure if there are some negative implications in there but it seems like our best bet.

@jaredcunha-usds
Copy link
Contributor Author

It'd be cool if you can submit a PR, but maybe if we update the file in the project and import that instead of uswds file? At least until they fix?

The only reason I have those scss files is to use the variables and function.s

@dmethvin-gov
Copy link
Contributor

@jaredcunha-usds I was going to make a PR but I can't find $number-to-value in the USWDS code. Can you find it?

@jaredcunha-usds
Copy link
Contributor Author

@dmethvin-gov is this what you're look for?

@dmethvin-gov
Copy link
Contributor

That's copied by the website setup from node_modules though, right? I can make a PR here in our website repo to change it but it will be clobbered when USWDS 2.0 arrives. I was looking for the place in USWDS. It only seems to appear in the /dist files, I can find it here in 2.0.0-beta6 but it looks totally different here in the develop branch.

@jaredcunha-usds
Copy link
Contributor Author

I think the first link you sent is closest to the right place. I don't know where they are taking PRs, but perhaps you can ask the TTS channel? I think that develop branch is a sort dev staging branch for work on v1.x.

@dmethvin-gov
Copy link
Contributor

dmethvin-gov commented Mar 29, 2019

Found it, I think. I'll make a PR against this and if I'm wrong I'll discuss with them there.
https://github.com/uswds/uswds/blob/release-2.0/src/stylesheets/core/_variables.scss

@dmethvin-gov
Copy link
Contributor

The fix should be in USWDS 2.0, yay! 🎉 uswds/uswds#3008

@jaredcunha-usds
Copy link
Contributor Author

jaredcunha-usds commented Mar 29, 2019 via email

update package and css overrides/changes

sort of working, swithching variables

update breakpoints

remove test accordion

updating css

remove uneccesary variables

create task to update without overwriting theme files

clean old gulp files

reorganize uswds src files, remove uneccessary files

update gulpfile and scss

package.json update
@jaredcunha-usds
Copy link
Contributor Author

@dmethvin-gov I think this is ready to merge now. I've removed some of the unused utilities, which saved around 50KB in CSS from the uswds.css file. I've made updates to the job application form in salesforce to support the USWDS changes. I was using pages/form.html to test that. I'll keep that in if you want to want to take a look, but I'll remove it before I merge this pull.

@jaredcunha-usds jaredcunha-usds changed the title DO NOT MERGE - Setting up for future USWDS updates Updating to USWDS 2.0.1 Apr 19, 2019
@dmethvin-gov
Copy link
Contributor

Thanks! I'll take a look! It's my first real look at USWDS 2.0 so it may take a day or two.

Copy link
Contributor

@dmethvin-gov dmethvin-gov left a comment

Choose a reason for hiding this comment

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

Wow, those were a lot of class renames!

What is your feeling on committing all the font binaries? Are they all coming from USWDS and only referenced from their [S]CSS? If so can we just have the build process copy them to the theme directory rather than putting them in the repo?

Are all the files in assets also copied from various dependencies, or are they "ours"? If they are unchanged from a dependency it would be nicer to just copy them there as with fonts. I don't know if Jekyll has a way to do that but can look into it if you want.


#### Update USDS and get new functions and tokens
1. Compile usds.css `gulp uswds-build-sass`
2. Run Jekylll `bundle exec jekyll serve`
Copy link
Contributor

Choose a reason for hiding this comment

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

Only two ls in Jekyll 😸

1. Update the USWDS version in `package.json`
2. Install the package `npm install`
2. Compile usds.css `gulp uswds-build-sass`
3. Run Jekylll `bundle exec jekyll serve`
Copy link
Contributor

Choose a reason for hiding this comment

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

Jekyll

README.md Outdated
## Adding Content
* [How to add people](https://github.com/usds/website/wiki/Adding-People-(carousel-and-pages))
* [How to add projects](https://github.com/usds/website/wiki/Adding-projects-(carousel-and-pages))
**Before you do this**: The USWS Sass uses many of the USWDS functions and tokens. While the USWDS.css is already compiled, the site relies on Jekyll to compile the Sass files. There is currently an issue with Jekyll the way it handles `.5` spacing tokens. [See here](https://github.com/uswds/uswds-sandbox/issues/18). **A fix has been merged, but still waiting to be deployed.**
Copy link
Contributor

Choose a reason for hiding this comment

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

Did this make it into 2.0.0 or 2.0.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.

Gah! It did it. I had this in two places. Must have missed that.

README.md Outdated
Use this to patch any display bugs through updates to USWDS.

1. Update the USWDS version in `package.json`
2. Install the package `npm install`
Copy link
Contributor

Choose a reason for hiding this comment

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

The first two steps can be done as npm install --save-dev *new-USWDS-version-number*

README.md Outdated
This will will update some of the scss files in `assets/uswds-sass`, but will not overwrite any of your files in `assets/uswds-theme`.

1. Update the USWDS version in `package.json`
2. Install the package `npm install`
Copy link
Contributor

Choose a reason for hiding this comment

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

npm install --save-dev *new-USWDS-version-number*

@@ -45,3 +62,5 @@
@import "utilities-add-ons/display";
@import "utilities-add-ons/flex";
@import "utilities-add-ons/position";

@import "shame/salesforce";
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this file supposed to be empty?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is part of that salesforce test form. I'm going to remove this before I merge it.

@@ -1,18 +1,20 @@
html {
-webkit-font-smoothing: antialiased;
-moz-osx-font-smoothing: grayscale;
font-size: 100%;
font-family: family('body');
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a much nicer way to do the font sizing!

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 still prefer this because it makes the math easier, but USWDS 2.0 doesn't seem to play as nicely with it.

html {
  font-size: 10px;
}

body {
  font-size: 1.7rem;
}

assets/stylesheets/components/_nav.scss Show resolved Hide resolved
assets/stylesheets/components/_who-we-help.scss Outdated Show resolved Hide resolved
assets/uswds-sass/core/_base.scss Show resolved Hide resolved
@jaredcunha-usds
Copy link
Contributor Author

@dmethvin-gov Yeah, this ended being a much larger effort than I anticipated, but it works! I tackled a few of your suggestions, but some of them were on files copied over via gulp from dependencies. Please see my responses for those.

What is your feeling on committing all the font binaries? Are they all coming from USWDS and only referenced from their [S]CSS? If so can we just have the build process copy them to the theme directory rather than putting them in the repo?

They're all moved over the gulp file. This is actaully from the USWDS team, but I'm open to changing at some point. I don't think this should affect performance, but it does make for a large commit.

Are all the files in assets also copied from various dependencies, or are they "ours"? If they are unchanged from a dependency it would be nicer to just copy them there as with fonts. I don't know if Jekyll has a way to do that but can look into it if you want.

It's a mix of "ours" and what's copied from a dependency. They are unchanged except for the uswds-theme files. Unfortunately, GitHub pages build process only allows for a plain Jekyll setup, so those files have be committed. I would really love to clean this up at some point, because it would make repo a heck of a lot cleaner.

@jaredcunha-usds jaredcunha-usds merged commit 8f4fe77 into master Apr 22, 2019
@lisac-usds lisac-usds deleted the uswds-update branch September 24, 2020 22:37
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.

None yet

2 participants