-
Notifications
You must be signed in to change notification settings - Fork 191
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
Move site nav into a menu on smaller screens #234
Conversation
Pages that are not responsive: * Component Name Matrix * Select Propsals * Checkbox Propsals Also, component name matrix is really slow to show menu. React is hogging the main thread. Also, the mobile menu won't show until the client-side React render happens, siiighh. This is blocked by a >1MB download of JS (mostly inlined images). siiggh
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.
If you have a chance, checkout the new nav on your phone so we can get some real device testing!
background-color: #666; | ||
} | ||
|
||
@media (max-width: 640px) { |
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 chose 640px semi-randomly (I just used the various device emulations devtools has to see what looked good on that sampling of devices). I imagine people here have done much more research on this so feedback here would be appreciated!
const ContentWrapper = | ||
frontmatter && frontmatter.path && frontmatter.path.startsWith('/components/') | ||
? ComponentLayout | ||
: ({ children }) => <>{children}</> |
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.
This line was effectively creating a new component on each render of Layout, causing React to rebuild the page each time the menu was toggled. Fixed it by just rendering children directly instead of wrapping them in a function.
Thanks for this @andrewiggins - I filed the following to track other mobile site improvements. Feel free to recommend any others you've seen: #239 |
On screens up to 640px wide, the site navigation is moved into a menu that is toggled on click. For this PR, I wanted to keep it very simple to serve as a stop-gap (as suggested in #170) until the proposed re-design in #29 happens.
Some current limitations I'll tackle in future PRs
I'll add some additional context to specific changes in the comments below.
Related: #170