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

Upgrade to Gatsby v2 #1104

Merged
merged 34 commits into from Sep 19, 2018

Conversation

Projects
None yet
9 participants
@alexandernanberg
Copy link
Contributor

commented Aug 1, 2018

Went through the site and followed the current migration guide.

In my local testing everything seems to be working, only problem I encountered was that the /blog/ redirect stopped working for some reason, looks like it's an issue inside Gatsby but not 100% sure on that one.

We might want to wait until Gatsby releases a RC or a stable release before merging this.

@facebook-github-bot

This comment has been minimized.

Copy link
Collaborator

commented Aug 1, 2018

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@facebook-github-bot

This comment has been minimized.

Copy link
Collaborator

commented Aug 1, 2018

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@reactjs-bot

This comment has been minimized.

Copy link

commented Aug 1, 2018

Deploy preview for reactjs ready!

Built with commit 59b6b1b

https://deploy-preview-1104--reactjs.netlify.com

@gaearon

This comment has been minimized.

Copy link
Member

commented Aug 1, 2018

There seems to be a difference in font sizes with https://reactjs.org/, at least for the homepage

@gaearon
Copy link
Member

left a comment

We need to fix "Blog" in top bar. Marking as needs changes so we don't accidentally forget

@alexandernanberg

This comment has been minimized.

Copy link
Contributor Author

commented Aug 1, 2018

Good catch, gatsby develop produces the right font family but gatsby build does not. I'll continue investigating!

@sw-yx

This comment has been minimized.

Copy link
Contributor

commented Aug 1, 2018

cc @kkemple and @jlengstorf, good candidate for pairing here

@jlengstorf

This comment has been minimized.

Copy link
Contributor

commented Aug 2, 2018

Hey, @alexandernanberg! If you'd like to set up a pair programming session to work through any of the migration, please let me know. I'm happy to jump on a screen share and dig through any tricky bits. (This also helps us improve the migration guide.)

Thanks!

@alexandernanberg

This comment has been minimized.

Copy link
Contributor Author

commented Aug 2, 2018

@jlengstorf I'd be down for that! Although I don't think there is really that much left, but as you said there were/are some tricky bits that I might need some guidance in

@jlengstorf

This comment has been minimized.

Copy link
Contributor

commented Aug 2, 2018

@alexandernanberg Great! Do any of these slots work for you? https://calendly.com/gatsbyjs-jason/30min

@alexandernanberg

This comment has been minimized.

Copy link
Contributor Author

commented Aug 2, 2018

@jlengstorf Yes, booked one that works for me!

@octalmage

This comment has been minimized.

Copy link

commented Aug 2, 2018

I believe the font-size issue is related to gatsbyjs/gatsby#6760, so I'm curious what you find.

@alexandernanberg

This comment has been minimized.

Copy link
Contributor Author

commented Aug 3, 2018

@octalmage Yep that's correct, although in this case it was possible to solve in another way. We were using glamor/reset (basically just a mirror of normalize.css) and by switching to use normalize.css directly the problem was resolved. Apparently you can easily run into problems when mixing different ways to do styling (css-in-js, css-modules etc)

@octalmage

This comment has been minimized.

Copy link

commented Aug 3, 2018

@alexandernanberg thanks for sharing! That makes sense, css is built in to Gatsby so it gets loaded first. Switching to normalize.css forces it to the top.

alexandernanberg added some commits Aug 3, 2018

@alexandernanberg

This comment has been minimized.

Copy link
Contributor Author

commented Aug 12, 2018

Looks like it's been there since the migration to Gatsby v1. @bvaughn looks like you were the one to add that, can you provide some context?

@bvaughn

This comment has been minimized.

Copy link
Contributor

commented Aug 13, 2018

@bvaughn looks like you were the one to add that, can you provide some context?

Nope, sorry. 😄 If it isn't still necessary, maybe it was at some earlier time? Gatsby changed a lot since we began using it. I don't remember anything specific about these few lines from 10+ months ago though.

@alexandernanberg

This comment has been minimized.

Copy link
Contributor Author

commented Aug 13, 2018

@bvaughn Okay, thanks anyway 😄

Removed that logic and looks like everything is working now. Would love for other people to try the site to verify this. The Netlify link can be found in the top of the comments!

Netlify build failed, but not on my local machine. Is it possible to purge the cache and rebuilt it? Oh, it was cache on my machine

alexandernanberg added some commits Aug 13, 2018

@gaearon

This comment has been minimized.

Copy link
Member

commented Aug 13, 2018

I noticed two small things:

  • The all-caps section headers in right side nav seem to look a bit different (font size? spacing?). I kinda like the new ones better.
  • When you go to Getting Started and click on any item in TOC, you'll notice we have something that adjusts the anchor scroll top after the navigation so that the top nav wouldn't obscure the header. The new website doesn't seem to have that logic (or it doesn't work). I think we'll want to have this.

Other than that, looks good to me

@alexandernanberg

This comment has been minimized.

Copy link
Contributor Author

commented Aug 14, 2018

Yeah, looks like there were a problem with specificity on the old site causing the font-family to be set to sans-serif instead of the system font stack. But that's fixed now since we are using normalize.css directly, hence why it looks different.

I've created a PR for the other problem you described. Was an issue with links that wasn't using the Link component from gatsby.

@gaearon

This comment has been minimized.

Copy link
Member

commented Aug 23, 2018

Is this ready to go in?

@alexandernanberg

This comment has been minimized.

Copy link
Contributor Author

commented Aug 24, 2018

Nope, not yet unfortunately. The bug with the links in the TOC is not solved (waiting on an upstream fix)

@alexandernanberg

This comment has been minimized.

Copy link
Contributor Author

commented Sep 15, 2018

@gaearon I think this should be ready now!

@gaearon gaearon merged commit 71b0348 into reactjs:master Sep 19, 2018

1 check passed

ci/circleci Your tests passed on CircleCI!
Details
@gaearon

This comment has been minimized.

Copy link
Member

commented Sep 19, 2018

Looks legit. Let's give it a go! Thank you ❤️

@bvaughn

This comment has been minimized.

Copy link
Contributor

commented Sep 19, 2018

😮 ❤️

@KyleAMathews

This comment has been minimized.

Copy link
Contributor

commented Sep 20, 2018

Yahoo! Thanks @alexandernanberg for your help here and fixes to Gatsby!

@alexandernanberg alexandernanberg deleted the alexandernanberg:gatsby-v2 branch Sep 21, 2018

@gaearon

This comment has been minimized.

Copy link
Member

commented Sep 26, 2018

Can you please look at this? #1201

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.