Skip to content

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

Merged
merged 4 commits into from
Mar 29, 2024

Conversation

Keshav-0907
Copy link
Contributor

Fixes #3035

Changes:

Created a useIsMobile hook where the breakpoint is defined and it returns a boolean value, the hook is also able to handle if any custom breakpoint is provided.

import { useMediaQuery } from 'react-responsive';

const useIsMobile = (customBreakpoint) => {
  const breakPoint = customBreakpoint || 770;
  const isMobile = useMediaQuery({ maxWidth: breakPoint });
  return isMobile;
};

export default useIsMobile;

I have verified that this pull request:

  • has no linting errors (npm run lint)
  • has no test errors (npm run test)
  • is from a uniquely-named feature branch and is up to date with the develop branch.
  • is descriptively named and links to an issue number, i.e. Fixes #3035

Copy link
Collaborator

@lindapaiste lindapaiste left a 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 ✔️

Comment on lines +17 to +19
{!isMobile && (
<Toolbar syncFileContent={props.syncFileContent} key={project.id} />
)}
Copy link
Collaborator

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

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();
Copy link
Collaborator

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.

@raclim raclim added Area: Mobile For issues affecting mobile or responsive behavior Area: Code Quality For refactoring, cleanup, or improvements to maintainability labels Mar 9, 2024
@raclim raclim added the Create Release Environment For use with ReleaseHub ephemeral environments label Mar 29, 2024
Copy link

release-com bot commented Mar 29, 2024

Release Environments

This Environment is provided by Release, learn more!
To see the status of the Environment click on Environment Status below.

🔧Environment Status : https://app.release.com/public/Processing%20Foundation/env-4ba9e03d9a

  • p5.js-web-editor

🔧Environment Status : https://app.release.com/public/Processing%20Foundation/env-8f588d3ea4

@raclim
Copy link
Collaborator

raclim commented Mar 29, 2024

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!

@raclim raclim merged commit 0df9082 into processing:develop Mar 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Code Quality For refactoring, cleanup, or improvements to maintainability Area: Mobile For issues affecting mobile or responsive behavior Create Release Environment For use with ReleaseHub ephemeral environments
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Centralising breakpoints
3 participants