-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Adopt Gatsby #2906
Adopt Gatsby #2906
Conversation
Close #1943
|
@mbrandau Are you up for giving this a try? |
When changing categories, seems to respond faster than the current site: http://shields-staging-pr-2906.herokuapp.com/ vs. |
Yeah I think I'm observing the same behavior, although in all honesty both are pretty snappy 😄 Two observations:
|
Thanks for taking this for a spin! Ah you're right, the favicon is broken.
Could you clarify this? |
Sure! I was switching between categories fairly quickly in both apps in an attempt to do an eyeball performance comparison. After some time of doing that in the current site (https://shields-staging.herokuapp.com/) the listings on some of the category pages appeared to not be rendering the badges (they eventually did render, but took 30+ seconds). So for a while, the listings only had the Badge example names and url's |
This PR has the same problem, or no? |
This PR does not have the same problem. Could've just been a fluke, but was only happening on https://shields-staging.herokuapp.com/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haven't done a full review on this as there is more to do, but this looks like we're heading in a good direction.
Can you give a bit more info on how will the routing work once we're deployed on GH pages? It seems like in order to do away with the client-side # style links, we need to add an extra build step to the deploy process to build static pages for each URL or something?
I've finished going through the files (my inline questions, etc. have already been addressed), and am just poking around in the PR staging app UI now. I just noticed one minor thing, probably off topic since this occurs in prod too, but when configuring a static badge after I select a color, part of the text of that color gets cut off if I pick a longer color (like bright green): |
Yea, that doesn't look great. That whole component could be improved. Maybe it could build off some of the work in the new builders. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is good to go, but there is a failing test to fix before we can merge
Great. Yea, that happened when I fixed the favicon. Will look into it… |
Looks like that fix worked. |
It's live! |
While Next.js can handle static sites, we've had a few issues with it, notably a performance hit at runtime and some bugginess around routing and SSR. Gatsby being fully intended for high-performance static sites makes it a great technical fit for the Shields frontend. The
createPages()
API should be a really nice way to add a page for each service family, for example.This migrates the frontend from Next.js to Gatsby. Gatsby is a powerful tool, which has a bit of downside as there's a lot to dig through. Overall I found configuration easier than Next.js. There are a lot of plugins and for the most part they worked out of the box. The documentation is good.
Links are cleaner now: there is no #. This will break old links though perhaps we could add some redirection to help with that. The only one I’m really concerned about
/#/endpoint
. I’m not sure if folks are deep-linking to the category pages.There are a lot of enhancements we could add, in order to speed up the site even more. In particular we could think about inlining the SVGs rather than making separate requests for each one.
While Gatsby recommends GraphQL, it's not required. To keep things simple and reduce the learning curve, I did not use it here.
Close #1943
Fix #2837 Fix #2616
To do before merging:
Test/fix Now deploy(not working, but let's not block)