-
-
Notifications
You must be signed in to change notification settings - Fork 302
refactor(suite): update layout size handling #19107
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
Conversation
534d74b
to
ef7fe94
Compare
β
Previously successful run of [Test] PR Suite Desktop e2e tests workflow has been found. |
03060df
to
f0149da
Compare
β
Previously successful run of [Test] PR Suite Desktop e2e tests workflow has been found. |
f0149da
to
50bd138
Compare
β
Previously successful run of [Test] PR Suite Desktop e2e tests workflow has been found. |
2 similar comments
β
Previously successful run of [Test] PR Suite Desktop e2e tests workflow has been found. |
β
Previously successful run of [Test] PR Suite Desktop e2e tests workflow has been found. |
ad77368
to
5fc51f0
Compare
β
Previously successful run of [Test] PR Suite Desktop e2e tests workflow has been found. |
@coderabbitai summary |
WalkthroughThis update refactors responsive layout and breakpoint handling across the codebase. It removes the legacy system that relied on window size categories, pixel thresholds, and custom media query utilities, replacing it with a new, theme-driven breakpoint system defined in a central π Recent review detailsConfiguration used: CodeRabbit UI π Files selected for processing (56)
β¨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. πͺ§ TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
5fc51f0
to
56ec2ff
Compare
β
Previously successful run of [Test] PR Suite Web e2e tests workflow has been found. |
β
Previously successful run of [Test] PR Suite Desktop e2e tests workflow has been found. |
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 like it very much! π
Just thinking if we can unify breakpoints, screen_query and useLayout to similar style and package. See my comment below
}: ResizableBoxProps) => { | ||
const resizableBoxRef = useRef<HTMLDivElement>(null); | ||
const rafRef = useRef<number | null>(null); |
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.
raf? royal air force? :D
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.
requestAnimationFrame:)
@@ -3,7 +3,7 @@ import { ReactNode } from 'react'; | |||
import styled from 'styled-components'; | |||
|
|||
import { Banner, Column } from '@trezor/components'; | |||
import { breakpointMediaQueries } from '@trezor/styles'; | |||
import { SCREEN_QUERY } from '@trezor/components/src/config/variables'; |
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.
nit: I suggest to import just the package:
import { variables } from '@trezor/components';
...
${variables.SCREEN_QUERY.ABOVE_DESKTOP} {
or export it explicitly in @trezor/components
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.
Also thinking about refactoring naming and structure without keeping in mind our previous state.
It would be really nice have everything on the same place and not import it from 3 different packages with different naming conventions:
SCREEN_QUERY
from@trezor/components
useLayoutSize
from@trezor/hooks
breakpoints
from@trezor/theme
Not sure if we are able to unify all of these or useLayoutSize must be somewhere else. In ideal world I would suggest to do something like this:
import { media } from '@trezor/???';
// media object
{
query: {
mobile: `@media ...`,
aboveMobile: `@media ...`,
...
}
breakpoints: {
mobile: 576,
table: 768,
...
}
useQuery: fn
}
WDYT?
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 don't want to touch SCREEN_QUERY as it's deprecated:) breakpoints are in theme to follow suit with other similar constants. useLayoutSize must be in hooks, because it depends on Redux.
}; | ||
default: | ||
return state; | ||
} | ||
}; | ||
|
||
export const ResizableBox = ({ |
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.
Good job here, this component needed some love. Reducer and 100% values look much better than before
[layoutSize], | ||
); | ||
// The guide should be on top for smaller screens (below laptop size) | ||
const isGuideOnTop = isBelowLaptop; |
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.
nit: I suggest isGuideOverlayContent
Why: I had to test it in the app because I though it does overlay content all the time. This seems to me more descriptive what's actually hapening. Up to you.
ππ₯οΈ Suite web test results: View in Currents
ππ₯οΈ Suite desktop test results: View in Currents