-
Notifications
You must be signed in to change notification settings - Fork 7
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
feat(datatrak): RN-1501: Offline screen #6032
base: dev
Are you sure you want to change the base?
Conversation
@@ -58,6 +59,7 @@ const AuthViewLoggedInRedirect = ({ children }) => { | |||
export const Routes = () => { | |||
return ( | |||
<RouterRoutes> | |||
<Route path="/offline" element={<OfflinePage />} /> |
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'm not 100% sure how the offline page will be used yet so I've just put it here
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.
Just nitpicks, but overall lgtm!
height: 100vh; | ||
`; | ||
|
||
const HeaderContainer = styled.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.
Is this semantically header?
const HeaderContainer = styled.div` | |
const HeaderContainer = styled.header` |
align-items: center; | ||
block-size: ${HEADER_HEIGHT}; | ||
padding: 0 1.5rem; | ||
margin: 0 auto; |
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 I’m not mistaken, margin-block
is already 0
. Best to set only what we need to override to clean up the cascade for debugging:
margin: 0 auto; | |
margin-inline: auto; |
width="100%" | ||
height="100%" |
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.
Correct me if React has some special sauce, but if this is a plain ol’ vanilla <img>
, then this must be a unitless integer. If my understanding is correct, these values as-is will be ignored
These are the intrinsic dimensions from the file:
width="100%" | |
height="100%" | |
width={84} | |
height={42} |
align-items: center; | ||
justify-content: center; | ||
text-align: center; | ||
padding: 1rem 1.5rem 1rem; |
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.
padding: 1rem 1.5rem 1rem; | |
padding-block: 1rem; | |
padding-inline: 1.5rem; |
margin-block-end: 3%; | ||
`; | ||
|
||
const Text = styled(Typography)` | ||
font-size: 1rem; | ||
margin-block-end: 5%; |
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.
Slightly concerned about these percentage values behaving unpredictably. Perhaps something like clamp()
might communicate our designed intent more clearly?
Issue #: feat(datatrak): RN-1501: Offline screen
Changes:
Screenshots: