Fix/Media library grid visually broken on mobile #53798
Conversation
- Add the getMediaScalePreference selector to return default mobile viewport media scale value if needed
… on mobile viewport - Use isMobile() function from @automattic/viewport to detect mobile viewport - Use getMediaScalePreference selector instead of getPreference selector for fetching media scale value from Redux
bluefuton
added
[Status] Needs Review
|
|
||
| if ( ( isMobile && mediaScale !== 1 ) || ( ! isMobile && mediaScale > SCALE_TOUCH_GRID ) ) { | ||
| return SCALE_TOUCH_GRID; | ||
| } |
There was a problem hiding this comment.
This condition deserves a comment that explains it. And maybe also being divided into two conditions:
- On mobile view, allow only 1 and 0.323 scales.
- On non-mobile view, don't allow the 1 scale: always show at least 3 columns.
And if the getMediaScalePreference selector ends up being called only at one place, let's inline it in that module. The selector is too specialized to be in the general-purpose state/preferences module.
| }; | ||
|
|
||
| static defaultProps = { | ||
| mediaValidationErrors: Object.freeze( {} ), | ||
| onAddMedia: noop, | ||
| source: '', | ||
| isMobile: isMobile(), |
There was a problem hiding this comment.
This default is not very useful in my opinion. It will be evaluated only once, when the app is loaded, and the result will be the default forever. And won't update when the window is resized.
There was a problem hiding this comment.
The value of isMobile should be retrieved by wrapping the component with the withMobileBreakpoint HOC. It will observe window size changes and will automatically pass an always-up-to-date isBreakpointActive prop to the wrapped component.
| @@ -364,6 +367,7 @@ export class MediaLibraryContent extends React.Component { | |||
| <MediaLibraryList | |||
| key="list-loading" | |||
| filterRequiresUpgrade={ this.props.filterRequiresUpgrade } | |||
| isMobile={ this.props.isMobile } | |||
There was a problem hiding this comment.
If we ended up drilling the isMobile prop from top down to the MediaLibraryScale component, can we use the mediaScale prop instead? That's the value we really want, isn't it?
That way, the mediaScale value will be computed only at one place, and passed down to all other components that need it.
There was a problem hiding this comment.
If we pass down mediaScale prop value to children components, I think they need to unsubscribe from the Redux Store because the mediaScale value from Redux would overwrite the value from the parent components. Would it be alright to unsubscribe?
There was a problem hiding this comment.
I'm not sure what you mean by "unsubscribe". Do you mean that the MediaLibraryScale preference would no longer read the scale prop in Redux with getMediaScalePreference, but it would get the value in the regular scale prop from the parent component? Then yes, that's alright and desirable. Ideally, there would be only one getMediaScalePreference call at the top of the tree and scale would be passed down, exactly the same way as your patch currently does with the isMobile prop.
richardkang112
force-pushed
the
fix/media-library-grid-visually-broken-on-mobile
branch
from
2de5556 to
e1bf204
Compare
richardkang112
requested review from
scinos,
worldomonation and
WunderBart
as code owners
richardkang112
force-pushed
the
fix/media-library-grid-visually-broken-on-mobile
branch
2 times, most recently
from
08b5869 to
e3db3e6
Compare
richardkang112
force-pushed
the
fix/media-library-grid-visually-broken-on-mobile
branch
from
e3db3e6 to
d955c3c
Compare
|
Sorry, I didn't mean to request review to bunch of people. I made some mistakes on Git. |
richardkang112
deleted the
fix/media-library-grid-visually-broken-on-mobile
branch
|
@jsnajdr Thank you very much for your feedback. They were crucial to solve the problem. 👍 |
Changes proposed in this Pull Request
Testing instructions
Please view http://calypso.localhost:3000/media/:domain on mobile width. The page should render 3 items per row on mobile viewport. You should be able to switch from 3 items per row to 1 item per row with the segmented control buttons. When you switch to a larger viewport (window screen width greater than 480) when the page was displaying 1 item per row on mobile viewport, the media scale value is set back to 0.323.
Fixes #48227
Related to #8295