-
Notifications
You must be signed in to change notification settings - Fork 0
Fix/tab swipe #409
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
Fix/tab swipe #409
Conversation
I pulled main into my branches after that merge and they ran the actions without any problems. I can't tell why this is failing at the moment. I'm triggering a re-build just to see if it's a fluke |
| import { PHONE_BREAKPOINT, TABLET_BREAKPOINT } from "../constants"; | ||
| import useWindowWidth from "./useWindowWidth"; | ||
|
|
||
| export const useMaxWidthBreakpoint = ( | ||
| breakpoint: number | ||
| ): boolean => { | ||
| const windowWidth = useWindowWidth(); | ||
| return windowWidth < breakpoint | ||
| }; | ||
|
|
||
| export const useMobileBreakpoint = () => useMaxWidthBreakpoint(PHONE_BREAKPOINT); | ||
| export const useTabletBreakpoint = () => useMaxWidthBreakpoint(TABLET_BREAKPOINT); | ||
|
|
||
| export default useMaxWidthBreakpoint; |
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 is how we are using the window width hook in most places, so might as well just make these available like this?
Could add some complexity to use different comparators if desired?
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.
love this util! only a minor thought: foldable phones might behave a bit differently, but that feels out of scope for now
I don’t see a preview QR code in the comments, but it should show up once this is pulled or merged into main. I’d also like to check orientation changes (e.g. vertical to horizontal) when we get a chance
| <div className={container}> | ||
| <div className={mobileTabBar}> | ||
| {tabsToRender.map((tabName) => ( | ||
| <button | ||
| key={tabName} | ||
| className={`${mobileTabButton} ${ | ||
| activeTab === tabName | ||
| ? mobileTabButtonActive | ||
| : "" | ||
| }`} | ||
| onClick={() => setActiveTab(tabName)} | ||
| > | ||
| {renderTabLabel(tabName)} | ||
| </button> | ||
| ))} | ||
| </div> | ||
| {tabComponents[activeTab]} | ||
| </div> |
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 is the swiping version, I think it's small enough to justify leaving here in this component?
| const renderTabLabel = (tabName: SubPage) => { | ||
| const label = tabName.split(" ").map((word) => { | ||
| return <span key={word}>{word}</span>; | ||
| }); | ||
| return ( | ||
| <Flex wrap justify="center" align="center" className={labelGroup}> | ||
| {label} | ||
| </Flex> | ||
| ); | ||
| }; |
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.
couldn't this be accomplished through CSS? what's the reason for this approach ?
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 is the same approach that was in the code before just factored out
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're asking why we are splitting the word into separate spans i don't actually remember how we landed at that approach, we can refactor if you want?
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.
that's a good enough answer, just curious
…talog into fix/tab-swipe
✅ Deploy Preview for cell-catalog ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Problem
Makes subpage tab buttons swipe on tablets and mobile.
Solution
Its fairly simple, most just relies on
overflow:autowhich works and relies on current styling.Carouselfrom antd seemed like a hassle?As a utility i wrapped the window width hook in a boolean return that checks breakpoints and defined a tablet and mobile hook, so far that's all we use
useWindowWidthfor, so we can just calluseTabletBreakpoint()oruseMobileBreakpointwhere needed. If we need to compare those values we can build on the idea.Added two lines in layout CSS module to prevent swiping from exposing components that are larger than the page, and to reduce margins.