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
Merged

Upgrade to Gatsby v2 #1104

merged 34 commits into from
Sep 19, 2018

Conversation

alexandernanberg
Copy link
Contributor

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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link

reactjs-bot commented Aug 1, 2018

Deploy preview for reactjs ready!

Built with commit 59b6b1b

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

@gaearon
Copy link
Member

gaearon commented Aug 1, 2018

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

Copy link
Member

@gaearon gaearon left a comment

Choose a reason for hiding this comment

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

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

@alexandernanberg
Copy link
Contributor Author

alexandernanberg commented Aug 1, 2018

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

@swyxio
Copy link
Contributor

swyxio commented Aug 1, 2018

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

@jlengstorf
Copy link
Contributor

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
Copy link
Contributor Author

@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
Copy link
Contributor

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

@alexandernanberg
Copy link
Contributor Author

@jlengstorf Yes, booked one that works for me!

@octalmage
Copy link

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

@alexandernanberg
Copy link
Contributor Author

@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
Copy link

@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.

@jlengstorf
Copy link
Contributor

If it works without it, that seems fine. Might be worth checking the blame to see who built this so you can ask for more context, but that's up to you and @gaearon.

@alexandernanberg
Copy link
Contributor Author

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
Copy link
Contributor

bvaughn 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
Copy link
Contributor Author

alexandernanberg 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

@gaearon
Copy link
Member

gaearon 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
Copy link
Contributor Author

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
Copy link
Member

gaearon commented Aug 23, 2018

Is this ready to go in?

@alexandernanberg
Copy link
Contributor Author

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

@alexandernanberg
Copy link
Contributor Author

@gaearon I think this should be ready now!

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

gaearon commented Sep 19, 2018

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

@bvaughn
Copy link
Contributor

bvaughn commented Sep 19, 2018

😮 ❤️

@KyleAMathews
Copy link
Contributor

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

@alexandernanberg alexandernanberg deleted the gatsby-v2 branch September 21, 2018 04:50
@gaearon
Copy link
Member

gaearon 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants