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

[WIP] Upgrades to USWDS 1.0.0 #5353

Closed
wants to merge 32 commits into from
Closed

[WIP] Upgrades to USWDS 1.0.0 #5353

wants to merge 32 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Apr 18, 2017

  • Updated package.json, npm-shrinkwrap.json
  • Removed Foundation JavaScript.
  • Removed Foundation grid, block-grid and visibility partials.
  • Removed jQuery references. Unneeded.
  • Updating body .row styles to compensate for Foundation removal.
  • Imports USWDS partials in correct order.
  • Removed b-functions (redundant).

- Updated package.json, npm-shrinkwrap.json
- Removed Foundation JavaScript.
- Removed Foundation grid, block-grid and visibility partials.
- Removed jQuery references. Unneeded.
- Updating body .row styles to compensate for Foundation removal.
- Imports USWDS partials in correct order.
- Removed b-functions (redundant).
@va-bot va-bot temporarily deployed to vetsgov-pr-5353 April 18, 2017 15:22 Inactive
@ghost ghost added the in progress label Apr 18, 2017
@ghost
Copy link
Author

ghost commented Apr 18, 2017

Not at all sure what's happening here. Here's what I did to create this PR:

  • Ran npm install --save-dev uswds@1.0.0 to update package.json
  • Deleted node_modules
  • Ran npm install to install whatever is in package.json
  • Ran npm run watch to verify that it works (it did, at least for me)
  • Ran npm run build to verify the verification (no errors).
  • Ran npm shinkwrap --dev to create npm-shrinkwrap.json

Running NODE_ENV=production npm run build -- --buildtype production doesn't give me errors locally either. But it throws all kinds of errors during the Jenkins build. Actually if I wait long enough, it does. Hrm.

@jbalboni
Copy link
Contributor

NODE_ENV=production npm run build -- --buildtype production does give me errors, which is weird. I got rid of some of them by putting this in style.scss on line 95:

$font-path: "~uswds/src/fonts"

But now I get errors from react-scroll.

@bshyong
Copy link
Contributor

bshyong commented Apr 18, 2017

^ Good catch on the font paths—I saw this error too when digging in on it. Overriding it that way does seem to fix the font path errors. The font path errors may have masked some underlying errors

@ghost
Copy link
Author

ghost commented Apr 18, 2017

Yeah, there's a lot of legacy weirdness in our code base. I don't think anyone has ever fully understood it at any point. So there's a lot of stuff that mostly works until you try to change it. 😞

@jbalboni
Copy link
Contributor

@tiffanybbrown-va Ok, I got it to work locally by making the font-path change above and downgrading react-scroll: npm install react-scroll@1.4.5 --save-dev. If you do those two things and then run shrinkwrap, I think the jenkins build will work (or at least make it to the e2e tests).

Tiffany Brown added 4 commits April 19, 2017 09:42
- Updated package.json, npm-shrinkwrap.json
- Removed Foundation JavaScript.
- Removed Foundation grid, block-grid and visibility partials.
- Removed jQuery references. Unneeded.
- Updating body .row styles to compensate for Foundation removal.
- Imports USWDS partials in correct order.
- Removed b-functions (redundant).
- Removes Bourbon, Neat from import path.
…-of-veterans-affairs/vets-website into update-uswds
@va-bot va-bot temporarily deployed to vetsgov-pr-5353 April 19, 2017 19:20 Inactive
@va-bot va-bot temporarily deployed to dev April 19, 2017 19:29 Inactive
@va-bot va-bot temporarily deployed to vetsgov-pr-5353 April 19, 2017 20:25 Inactive
@va-bot va-bot temporarily deployed to dev April 19, 2017 20:40 Inactive
Tiffany Brown added 4 commits April 19, 2017 13:51
- Updated package.json, npm-shrinkwrap.json
- Removed Foundation JavaScript.
- Removed Foundation grid, block-grid and visibility partials.
- Removed jQuery references. Unneeded.
- Updating body .row styles to compensate for Foundation removal.
- Imports USWDS partials in correct order.
- Removed b-functions (redundant).
- Removes Bourbon, Neat from import path.
@ghost
Copy link
Author

ghost commented Apr 19, 2017

Importing uswds.scss and locking react-scroll to version 1.4.5 seems to have fixed some errors. Now need to add the updated USWDS JavaScript and change some markup :-/

@jbalboni
Copy link
Contributor

@tiffanybbrown-va I think you should be able to remove the npm-shrinkwrap.json file in here and do yarn upgrade uswds@1.0.0 to make this PR work with the Yarn change I just merged.

@va-bot va-bot temporarily deployed to dev April 20, 2017 20:12 Inactive
@va-bot va-bot temporarily deployed to vetsgov-pr-5353 April 28, 2017 16:09 Inactive
@rroueche rroueche assigned ghost Apr 28, 2017
@ghost ghost removed the website label May 4, 2017
@jbalboni
Copy link
Contributor

jbalboni commented May 8, 2017

@tiffanybbrown-va I'm happy to help with whatever we need to get this item merged or in a good place when you leave.

@ghost
Copy link
Author

ghost commented May 10, 2017

Not sure why this Jenkins build keeps failing. Running yarn run heroku-postbuild builds locally for me without errors.

@ghost
Copy link
Author

ghost commented May 11, 2017

Ah. A failing test: .usa-accordion-content. This is likely because the newer USWDS markup and JavaScript hasn't been added yet.

@annekainicUSDS
Copy link
Contributor

@tiffanybbrown-va @jbalboni @bshyong after reviewing this PR and chatting with Tiffany on the phone, I'm wondering if it would make things slightly simpler if we reduce the scope of this PR to simply be updating USWDS, and not attempt to remove Foundation at this point. Then we can strip out Foundation app by app, until eventually we are no longer using Foundation classes and can feel confident about removing it entirely.

It sounds like there are a few changes that would have to remain in this PR:

  1. Fixing the order of the file imports
  2. Converting the base font size to px from rem
  3. Including some CSS that USWDS doesn't account for

Perhaps some additional things would have to be added too:

  1. Importing USWDS Javascript

The reason for recommending this approach is that given the list of things that are still to do in order to complete this task (see Tiffany's notes here), this PR could get very big and will take awhile to merge. Trying to think of a way to break it down into pieces that are more manageable and can be merged more frequently so we don't get into the drift issue we've had so far.

Not sure if this is actually possible because I haven't played around with this as much as you all have, so let me know if there's anything I'm missing! General thoughts?

@ghost
Copy link
Author

ghost commented May 12, 2017

Wrote up some hand-off notes that summarizes what I think is left to do here.

…-of-veterans-affairs/vets-website into update-uswds
@annekainicUSDS
Copy link
Contributor

annekainicUSDS commented May 12, 2017

I started a new branch from master and tried the above approach. I tried upgrading USWDS without taking out Foundation. I started to get a ton of build failures, primarily due to trying to multiply 2 different variables together from 2 different USWDS partials that had incompatible units. If I then removed the Foundation imports, those build failures went away. I have no idea how importing Foundation contribute to this problem, or why this problem was even happening, but without digging into this further I wonder if there will inevitably be conflicts in moving to USWDS without stripping out Foundation, which is why this PR is where it's at now.

Since my suggested approach does not seem to be working, we may want to continue with this PR. The only concerns I have with continuing this PR in its current state are:

  1. We're getting an odd yarn install error, both locally and in Jenkins, that I can't figure out the cause of.
  2. There is still much more to do for this PR to be complete, which means it's going to be super big, which makes things hard to review.
  3. The only way I can see maintaining this large PR that might still take some time to complete is to constantly keep it updated with master. Because we've been merging master in, it's making the git commit history pretty messy. I would prefer to be rebasing daily with master to keep it up to date until we can merge it. I started a rebase, but it took me down a long path of merge conflicts that I abandoned. It might be worth getting through those to keep the commit history clean.

One other option is to put this PR on hold, submit smaller PRs for removing all Foundation classes across the site and replacing with USWDS classes, and then leaving the upgrade and the final removal of Foundation until all Foundation use in vets-website is totally gone.

Looking for some other front-end developers to weigh in here before deciding on the best approach.

Kudos to @tiffanybbrown-va for tackling this messy problem. It is a case of working through issue after issue, which is hard work to do. You're leaving this to us in a much better state than it was in before!

@patrickvinograd
Copy link
Contributor

Obsolete in favor of other USWDS work, closing.

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

6 participants