-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Centralised Breakpoints using useIsMobile hook #3042
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
Centralised Breakpoints using useIsMobile hook #3042
Conversation
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.
@raclim I didn't run the branch but the code is good ✔️
{!isMobile && ( | ||
<Toolbar syncFileContent={props.syncFileContent} key={project.id} /> | ||
)} |
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.
So much cleaner!
<MediaQuery maxWidth={770}> | ||
{(mobile) => renderContent(currentTab, params.username, mobile)} | ||
</MediaQuery> | ||
{renderContent(currentTab, params.username, isMobile)} |
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.
FYI as things evolve I'm hoping that we can eventually use the useIsMobile
hook directly in the child components instead of passing down the prop. Def good for now though!
// TODO: centralize breakpoints | ||
const isMobile = useMediaQuery({ maxWidth: 770 }); | ||
|
||
const isMobile = useIsMobile(); |
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.
Looking at these lines you can see why this PR is so important! The other components with MediaQuery
are using minWidth={770}
. I wrote the code on the left and used maxWidth: 770
. But it should have been maxWidth: 669
so that it looks right when the screen is exactly 770px wide.
Release EnvironmentsThis Environment is provided by Release, learn more! 🔧Environment Status : https://app.release.com/public/Processing%20Foundation/env-4ba9e03d9a
🔧Environment Status : https://app.release.com/public/Processing%20Foundation/env-8f588d3ea4 |
I just ran the branch and it seems good to me so far! I also created a release environment for this to make accessing it from mobile a bit easier too (this should be the URL to go to on mobile: app-ted261d.release.editor.p5js.org/). Since I couldn't spot anything I'm going to merge this in, but let me know if there's anything else! |
Fixes #3035
Changes:
Created a
useIsMobile
hook where the breakpoint is defined and it returns aboolean
value, the hook is also able to handle if any custom breakpoint is provided.I have verified that this pull request:
npm run lint
)npm run test
)develop
branch.Fixes #3035