Add Mobile And Table Screens#1562
Add Mobile And Table Screens#1562bedeho merged 7 commits intoJoystream:init_atlasfrom fcescob:responsive-screens
Conversation
| const Container = styled.div<GridProps>` | ||
| const toPx = (n: number | string) => (typeof n === 'number' ? `${n}px` : n) | ||
|
|
||
| const LARGE_VIEWPORT_BREAKPOINT = toPx(2000) |
There was a problem hiding this comment.
Maybe let's move that to theme.breakpoints so we don't have a bunch of breakpoints scattered across the source files
| } | ||
| }, | ||
| }) | ||
| const isLargeViewport = useMediaQuery({ query: `(min-width: ${LARGE_VIEWPORT_BREAKPOINT})` }) |
There was a problem hiding this comment.
Why not use a regular CSS media query here, without JS?
There was a problem hiding this comment.
For some reason it was not working properly when I tried, but now looks fine, so I removed this and went back to regular CSS.
|
|
||
| export const CoverImage = styled.img<CoverImageProps>` | ||
| display: block; | ||
| position: absolute; |
There was a problem hiding this comment.
I don't think the image should be absolute here. What's stopping us from having it be a part of the layout?
There was a problem hiding this comment.
I'm setting the image to be absolute because I'm using a "trick" to preserve the aspect ratio of the images.
I absolutely position each image in a div which has a width of 100% and a padding-top of 100% * aspectRatio, so the absolute positioning is required in this case.
I'm happy to change this if there is a better way though.
| display: flex; | ||
| align-items: center; | ||
| padding-top: ${theme.sizes.b10}px; | ||
| @media (max-width: ${theme.breakpoints.mobile}) { |
There was a problem hiding this comment.
I see we both made improvements to that view, that's probably my bad. I added a couple more things though I think so let's get this merged and I'll rebase
There was a problem hiding this comment.
Let's leave this as is then and I'll leave it up to you to rebase / add your changes on top.
| export const Content = styled.div` | ||
| display: grid; | ||
| grid-template-columns: 650px 1fr; | ||
| grid-template-columns: minmax(50%, 650px) 1fr; |
There was a problem hiding this comment.
This isn't related to this piece of code. But the best match video still suffers from the problem of changing the image aspect ratio when resizing. Also take a look at how it looks like at 500px, this needs improvement:
There was a problem hiding this comment.
Didn't catch that, thanks. I've made some changes and now it should look better. The trick to preserve the aspect ratio I've described above is used here as well, again happy to chance if a better solution comes up.
| grid-template-columns: min(650px, 50%) 1fr; | ||
| grid-column-gap: 24px; | ||
|
|
||
| @media (max-width: 750px) { |
There was a problem hiding this comment.
Shouldn't that use one of the defined breakpoints?
| small: '21rem', | ||
| medium: '48rem', | ||
| large: '72rem', | ||
| mobile: '480px', |
There was a problem hiding this comment.
I've seen you've renamed large, maybe we should rename the others as well at the same time?
| @@ -1,5 +1,6 @@ | |||
| import styled from '@emotion/styled' | |||
There was a problem hiding this comment.
Relating to the styling since I can't comment the specific line:
Looks much better now, but I think we should also tweak the padding of InnerContainer once the title moves underneath the poster. Also the Container has right padding that doesn't seem to fit in mobile view. Take a look:
There is excess white space to the right and between the poster and the title
No description provided.