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

Replace hidden-xs class with an isMobile check #54791

Closed
huyenltnguyen opened this issue May 14, 2024 · 2 comments · Fixed by #54737
Closed

Replace hidden-xs class with an isMobile check #54791

huyenltnguyen opened this issue May 14, 2024 · 2 comments · Fixed by #54737
Labels
platform: learn UI side of the client application that needs familiarity with React, Gatsby etc. scope: UI Threads for addressing UX changes and improvements to user interface status: PR in works Work in Progress (WIP) Issues.

Comments

@huyenltnguyen
Copy link
Member

Description

We are using the hidden-xs class to conditionally hide some text on mobile screens:

<span className='hidden-xs'>
{isMacOS ? ' (Command + Enter)' : ' (Ctrl + Enter)'}
</span>

This class comes from Bootstrap, which is something we are going to remove (#52030). So instead of using the class, we can use an isMobile check, like this (which is also a little more consistent with how we've handled this kind of conditional rendering):

const isMobile = useMediaQuery({
query: `(max-width: ${MAX_MOBILE_WIDTH}px)`
});

Requirements

Update the completion-modal file to replace the hidden-xs class with an isMobile check.

Notes

I noticed that we are using multiple methods for mobile device detection, so I think the check should be standardized, and there should be a shared util for this check. But I'm leaving this out of the scope of this issue, and will create a separate issue for it.

@huyenltnguyen huyenltnguyen added help wanted Open for all. You do not need permission to work on these. scope: UI Threads for addressing UX changes and improvements to user interface platform: learn UI side of the client application that needs familiarity with React, Gatsby etc. labels May 14, 2024
@camperbot
Copy link
Contributor

We typically do not assign issues. Instead, we accept the first pull request that comprehensively solves the issue.

Issues labelled with help wanted or first timers only are open for contributions.

Please make sure you read our guidelines for contributing. We prioritize contributors following the instructions in our guide. Join us in our chat room or the forum if you need help contributing - our community will be happy to assist you.

@a2937
Copy link
Member

a2937 commented May 14, 2024

@huyenltnguyen I already have a pull request that technically resolves this. While it is done the same way we did the lower-jaw i.e checks for a desktop instead of being mobile , it does solve the problem and pass the tests.

#54737

@ilenia-magoni ilenia-magoni added status: PR in works Work in Progress (WIP) Issues. and removed help wanted Open for all. You do not need permission to work on these. labels May 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
platform: learn UI side of the client application that needs familiarity with React, Gatsby etc. scope: UI Threads for addressing UX changes and improvements to user interface status: PR in works Work in Progress (WIP) Issues.
Projects
None yet
4 participants