style(datatrakWeb): RN-1448: Responsive styling for landing page #5938
Conversation
There was a problem hiding this comment.
Have to admit I’ve mostly reviewed for readability; haven’t had the chance to pull the code and run it because I’m a bit time-constrained this week (and been a while since I last ran Tupaia)
Looks really great overall! A flurry of suggestions for possible minor improvements from me—the biggest one being how to handle components that have a mobile and desktop version. Otherwise mostly trivial details, so pre-approving 🍾
| const ButtonContent = styled.div` | ||
| display: flex; | ||
| flex-direction: column; | ||
| width: 100%; |
There was a problem hiding this comment.
Far from critical, there are a couple places in this file where logical properties would be more appropriate
| width: 100%; | |
| inline-size: 100%; |
| tbody tr:last-child { | ||
| td { | ||
| padding-block-end: 0.5rem; | ||
| } | ||
| } |
There was a problem hiding this comment.
| tbody tr:last-child { | |
| td { | |
| padding-block-end: 0.5rem; | |
| } | |
| } | |
| tbody tr:last-child td { | |
| padding-block-end: 0.5rem; | |
| } |
| export const DesktopHeaderLeft = ({ onClickLogo }) => { | ||
| return ( | ||
| <Logo component={RouterLink} onClick={onClickLogo} to="/" title="Home"> | ||
| <img src="/datatrak-logo-black.svg" alt="Tupaia Datatrak logo" width="100%" height="100%" /> |
There was a problem hiding this comment.
Is it worth inlining this SVG to save an HTTP request?
(I could easily be convinced “no”. I do personally like the readability of this, but inlining is an easy performance win 🤷)
| top: 0; | ||
| left: 0; |
There was a problem hiding this comment.
| top: 0; | |
| left: 0; | |
| inset-block-start: 0; | |
| inset-inline-start: 0; |
| <Wrapper> | ||
| <ActivityFeedList items={firstPageItems} /> | ||
| {hasNextPage && ( | ||
| <ViewMoreButton onClick={() => setExpanded(true)}>View activity feed...</ViewMoreButton> |
There was a problem hiding this comment.
| <ViewMoreButton onClick={() => setExpanded(true)}>View activity feed...</ViewMoreButton> | |
| <ViewMoreButton onClick={() => setExpanded(true)}>View activity feed…</ViewMoreButton> |
| > section { | ||
| &:not(:last-child) { | ||
| margin-bottom: 0; | ||
| } | ||
| } |
There was a problem hiding this comment.
| > section { | |
| &:not(:last-child) { | |
| margin-bottom: 0; | |
| } | |
| } | |
| > section &:not(:last-child) { | |
| margin-bottom: 0; | |
| } |
| export const ResponsiveScrollBody = styled.div` | ||
| display: grid; | ||
| overflow-x: auto; | ||
| grid-template-columns: repeat(auto-fill, minmax(12rem, 18rem)); |
There was a problem hiding this comment.
| grid-template-columns: repeat(auto-fill, minmax(12rem, 18rem)); | |
| grid-template-columns: repeat(auto-fill, minmax(max(100%, 12rem), 18rem)); |
| View more | ||
| </DesktopButton> | ||
| <MobileButton component={Link} to={ROUTES.TASKS}> | ||
| View more... |
There was a problem hiding this comment.
| View more... | |
| View more… |
| @@ -107,9 +144,9 @@ export const TasksSection = () => { | |||
| <FlexSpaceBetween> | |||
| <SectionHeading>My tasks</SectionHeading> | |||
| {hasTasks && ( | |||
| <ViewMoreButton component={Link} to={ROUTES.TASKS} display={{ xs: 'none', sm: 'block' }}> | |||
| <TopViewMoreButton component={Link} to={ROUTES.TASKS}> | |||
| View more... | |||
There was a problem hiding this comment.
| View more... | |
| View more… |
merge: update branch with latest dev
Issue RN-1448: Responsive styling for landing page
Changes:
Screenshots: