[RNMobile] Gallery - GalleryImage component #18155
Conversation
mkevins
added
[Status] In Progress
mkevins
changed the title
| @@ -0,0 +1,95 @@ | |||
| .container { | |||
| flex: 1; | |||
| height: 150px; | |||
There was a problem hiding this comment.
Do we have plans to make this dynamic in another PR?
When I tested the tiles component, I used mostly portrait images and it felt odd that they were cropped on the sides. I think the web implementation is currently better at maintaining aspect ratio when possible, and cropping consistently (our tiles crop things differently depending on position)
BTW, I think the current layout would be perfectly acceptable for a v1, but I want to make that decision explicit. cc @iamthomasbishop
There was a problem hiding this comment.
I would agree, the way that the web handles cropping is better in terms of honoring aspect ratios. Obviously, the sooner we can iterate to match this behavior, the better, but I wouldn't consider it a blocker for a first iteration.
There was a problem hiding this comment.
Thanks for testing this out Koke.
Do we have plans to make this dynamic in another PR?
I'm glad you asked about that, as I've been contemplating how best to address the aspect ratio concern. For simplicity in this iteration, I've "hard-coded" the height here to a somewhat arbitrary value. I've been wondering what the optimal solution for mobile would be, and in particular, the best way to utilize a common Image component that better respects aspect ratio.
It seems that the web implementation relies on the browser to set the height of an image based on its width. One concern I have in mind for mobile is the row height adjustment that happens as a result of the image ordering. On web, when I move a portrait image to a "more crowded" row, its width is reduced, reducing its height, potentially altering the height of the row it was in, or the row it was moved to. Here's an example:
When this happens on mobile, I wonder whether this "unpredictability" of row heights would affect the UX, since the position of the mover buttons may change. Another consideration, is that image dimensions may not be known until later, so the row size adjustments can happen in a "laggy" way once the image has been loaded over the network. In any case, I think the cross-platform Image component can be very useful for whatever route we take.
There was a problem hiding this comment.
I'm glad you asked about that, as I've been contemplating how best to address the aspect ratio concern. For simplicity in this iteration, I've "hard-coded" the height here to a somewhat arbitrary value. I've been wondering what the optimal solution for mobile would be, and in particular, the best way to utilize a common Image component that better respects aspect ratio.
MediaUploadProgress calculates the the height according to the container width actually. But we should be able to set the container width first. This is how it works on Image block as well, as far as I recall... When I check web implementation there's some logic that manually calculates the width of every tile in css.
// On mobile and responsive viewports, we allow only 1 or 2 columns at the most.
& .blocks-gallery-image,
& .blocks-gallery-item {
width: calc((100% - #{ $grid-size-large }) / 2);
// Beyond mobile viewports, we allow up to 8 columns.
@include break-small {
@for $i from 3 through 8 {
&.columns-#{ $i } .blocks-gallery-image,
&.columns-#{ $i } .blocks-gallery-item {
width: calc((100% - #{ $grid-size-large } * #{ $i - 1 }) / #{ $i });
margin-right: 16px;
// Rules inside this query are only run by Microsoft Edge.
// Edge miscalculates `calc`, so we have to add some buffer.
// See also https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/15637241/
@supports (-ms-ime-align:auto) {
width: calc((100% - #{ $grid-size-large } * #{ $i - 1 }) / #{ $i } - 1px);
}
}
}
We should put a similar logic in JS side, when screen size is less than a certain amount we should limit the displayed column count by 2, for bigger screens we can let up to 8 as web does. At the end it looks to me like we need to calculate the width of every tile on JS side manually.
There was a problem hiding this comment.
Hi @pinarol 👋
MediaUploadProgresscalculates the the height according to the container width
I understand this logic lives in the calculatePreferedImageSize function within ImageSize, imported as a local dependency of MediaUploadProgress. This seems to only be used when coverUrl is set, and relies on the onLayout prop of its wrapping container View.
When I check web implementation there's some logic that manually calculates the width of every tile in css.
In the web implementation, width is calculated in scss, but the css-calculated height property seems to be auto for the inner-most relevant container (i.e. the image and some containers may have height set to 100%, but ultimately, the height is from that auto value). My concern around mimicking this on mobile is that the row heights will be varied, and possibly "unpredictable". In the gif above, you can see an example of this behavior. Also, you can see there that the aspect ratio of the pelicans image is not honored very well when it's in the row with more images.
We should put a similar logic in JS side, when screen size is less than a certain amount we should limit the displayed column count by 2, for bigger screens we can let up to 8 as web does. At the end it looks to me like we need to calculate the width of every tile on JS side manually.
In the current state of this first iteration, this calculation is happening in the GalleryEdit, Gallery, and Tiles components GalleryEdit limits the columns via the MAX_COLUMNS constant, Gallery limits the columns to 2 when screen width is small via the displayedColumns variable, and Tiles calculates the width of each tile to allow spacing between and wrapping. Shall the individual images be responsible for some of this behavior instead? Or is this a general comment not specific to this component?
There was a problem hiding this comment.
They are in fact stretching to fill the flex container — the tallest item is defining the height of that.
I understand that they can stretch. But what is the tallest one in our case? The tallest one in gallery? Because all the tiles are in a single container.
Difference on web is they are stretching to fill the container row by row, as if there are different rows, but actually there aren't. That's the thing I don't get.
There was a problem hiding this comment.
If there's a simple way of doing this I'd really want to do this in this PR, we don't have to wait for the the cross platform Image component. MediaUploadProgress can tell you the width and height if we pass it the coverUrl prop.
There was a problem hiding this comment.
Difference on web is they are stretching to fill the container row by row, as if there are different rows, but actually there aren't. That's the thing I don't get.
That is exactly what flexbox is doing. From the spec, flex-wrap: wrap means the flex container is multi-line. And multi-line containers:
Once content is broken into lines, each line is laid out independently; flexible lengths and the justify-content and align-self properties only consider the items on a single line at a time.
In a multi-line flex container (even one with only a single line), the cross size of each line is the minimum size necessary to contain the flex items on the line (after alignment due to align-self), and the lines are aligned within the flex container with the align-content property. In a single-line flex container, the cross size of the line is the cross size of the flex container, and align-content has no effect. The main size of a line is always the same as the main size of the flex container’s content box
I think this is also possible on Yoga, and Image component might be our only issue there, as this automatic sizing relies on the image being able to calculate its intrinsic size. Flexbox can't adapt to "the tallest one", if the items can't tell their own size.
There was a problem hiding this comment.
Once content is broken into lines, each line is laid out independently;.
Aha this explains the web part better, thanks! I hope same thing works on mobile, because I don't see any row containers when I inspect the view hierarchy on XCode.
Flexbox can't adapt to "the tallest one", if the items can't tell their own size.
Our images can tell their own sizes if we use MediaUploadProgress with coverUrl, but it will take a bit of time until their real aspectRatio is calculated. So we should put a kind of placeholder with a constant height for each image until we get the real aspect ratio from the url. @mkevins could you try this?
There was a problem hiding this comment.
Hi @pinarol
Our images can tell their own sizes if we use MediaUploadProgress with coverUrl
I've tried using this before and encountered a few issues that seem like blockers (i.e. using the sizes calculated from ImageSize via the function as children approach). One issue is that the logic used assumes dimensions are numbers, and not strings / percentages, so it may not be working as expected (e.g. '100%' < '42%' === true 🙈 ). Another issue is that the additional views in the hierarchy may be interfering with the layout and resizeMode style.
Try with coverUrl
diff --git a/packages/block-library/src/gallery/gallery-image.native.js b/packages/block-library/src/gallery/gallery-image.native.js
index 666a408..64e0bb2 100644
--- a/packages/block-library/src/gallery/gallery-image.native.js
+++ b/packages/block-library/src/gallery/gallery-image.native.js
@@ -135,7 +135,7 @@ class GalleryImage extends Component {
const { compose } = StyleSheet;
const { isUploadInProgress } = this.state;
- const { isUploadFailed, retryMessage } = params;
+ const { isUploadFailed, retryMessage, finalWidth, finalHeight } = params;
const resizeMode = isCropped ? 'cover' : 'contain';
const imageStyle = [ styles.image, { resizeMode },
@@ -153,7 +153,7 @@ class GalleryImage extends Component {
return (
<>
<Image
- style={ imageStyle }
+ style={ [ imageStyle, { width: finalWidth, height: finalHeight } ] }
source={ { uri: url } }
// alt={ alt }
accessibilityLabel={ ariaLabel }
@@ -221,7 +221,7 @@ class GalleryImage extends Component {
}
render() {
- const { id, isBlockSelected, onRemove } = this.props;
+ const { id, isBlockSelected, onRemove, url } = this.props;
return (
<TouchableWithoutFeedback
@@ -230,7 +230,7 @@ class GalleryImage extends Component {
>
<View style={ styles.container }>
<MediaUploadProgress
- // coverUrl={ url }
+ coverUrl={ url }
mediaId={ id }
onUpdateMediaProgress={ this.updateMediaProgress }
onFinishMediaUploadWithSuccess={ this.finishMediaUploadWithSuccess }also tried:
@@ -158,6 +158,8 @@ class GalleryImage extends Component {
// alt={ alt }
accessibilityLabel={ ariaLabel }
ref={ this.bindContainer }
+ width={ finalWidth }
+ height={ finalHeight }
/>
{ isUploadFailed && (
<View style={ styles.uploadFailedContainer }>Screenshot
Technically we already have the aspect ratio, MediaUploadProgress can do the same thing. The cross platform Image component brings better architecture but doesn't solve the problem about setting different heights for tiles in different perceived rows.
The logic used in MediaUploadProgress isn't quite the same thing, since it requires pixel-based width, which we don't have in this case. The row heights are solved nicely by Flexbox, so we shouldn't need anything custom for handling that.
Using Koke's cross-platform Image component seems to be promising for handling the aspect ratio issue: work in progress patch, but there are a few things to figure out for that (e.g. alignment within the container, passing resizeMode, etc.)
Screenshot
We can try but I think this deserves a separate iteration on mobile. And I agree, for the first iteration I'd be ok with the current state.
If there's a simple way of doing this I'd really want to do this in this PR
I'm not seeing a simple solution, so maybe the best course is to defer to a later PR. WDYT?
|
I did some tests for selecting from WPMedia library, it is generally working pretty good 🎉 There are some issues pretty noticeable so we might want to address them:
I think there are multiple things to do about this. We can choose another screen width threshold for limiting the max columns. This is how it looks with 4 and 5 columns so we can just allow max of 4 or 5 if the screen width is smaller than X(not sure exactly what X should be, needs experimenting):
Even in the case of 5 columns the buttons look a bit odd, we might need to still shrink them a bit? Web also shrinks the padding if there's more than 6 columns, we can do sth similar.
Maybe we should use a different background color similar to web:
These items are mostly related with designs so we should get help from @iamthomasbishop |
|
Thanks @pinarol for testing this out and documenting these issues! 😃
For this one, I was wondering the best way to handle the margin. One way I had in mind was to allow passing style to the Use style prop in Tiles componentdiff --git a/packages/block-library/src/gallery/gallery.native.js b/packages/block-library/src/gallery/gallery.native.js
index e24a717..36841ad 100644
--- a/packages/block-library/src/gallery/gallery.native.js
+++ b/packages/block-library/src/gallery/gallery.native.js
@@ -1,7 +1,7 @@
/**
* External dependencies
*/
-import { View } from 'react-native';
+import { StyleSheet, View } from 'react-native';
/**
* Internal dependencies
@@ -15,6 +15,8 @@ import Tiles from './tiles';
*/
import { __, sprintf } from '@wordpress/i18n';
+const TILE_SPACING = 15;
+
export const Gallery = ( props ) => {
const {
selectedImage,
@@ -44,8 +46,9 @@ export const Gallery = ( props ) => {
<View>
<Tiles
columns={ displayedColumns }
- spacing={ 15 }
- align={ align }
+ spacing={ TILE_SPACING }
+ style={ { marginBottom: isSelected ? TILE_SPACING : 0 } }
+ // align={ align }
>
{ images.map( ( img, index ) => {
/* translators: %1$d is the order number of the image, %2$d is the total number of images. */
diff --git a/packages/block-library/src/gallery/tiles.native.js b/packages/block-library/src/gallery/tiles.native.js
index 1c1ed77..cbbd7e3 100644
--- a/packages/block-library/src/gallery/tiles.native.js
+++ b/packages/block-library/src/gallery/tiles.native.js
@@ -1,7 +1,7 @@
/**
* External dependencies
*/
-import { View } from 'react-native';
+import { View, StyleSheet } from 'react-native';
/**
* WordPress dependencies
@@ -18,8 +18,11 @@ function Tiles( props ) {
columns,
children,
spacing = 10,
+ style,
} = props;
+ const { compose } = StyleSheet;
+
const tileCount = Children.count( children );
const lastTile = tileCount - 1;
const lastRow = Math.floor( lastTile / columns );
@@ -66,8 +69,10 @@ function Tiles( props ) {
);
} );
+ const containerStyle = compose( styles.containerStyle, style );
+
return (
- <View style={ styles.containerStyle }>
+ <View style={ containerStyle }>
{ wrappedChildren }
</View>
);Alternatively, we could modify
I like your idea of using another threshold, and that should be pretty straightforward I think using the
This one puzzles me, since our
Using a background like web might be nice.. it's interesting that on Android, the default is transparent background, but white for iOS. I think the size problem may warrant a separate issue, unless there's a quick solution (I hadn't yet given that much thought). Should all the issues described here be solved / fixed for this iteration, or some of them? I can open issues for them to track and prioritize, if that makes sense. |
Sounds OK to me. We can set the
I don't mean to put the MediaPlaceholder component for each tile, I just mean to set a kind of background color(maybe with an icon similar to image/video blocks?) that will just indicate that there's something there until the image is downloaded from url.
I am sure @iamthomasbishop can enlighten us about this :)
Let's try to solve them in this PR, most seem like quick wins with a bit of a design guidance and they are mostly kind of cases that cause gallery seem buggy. But this one is experimental, let's try it and see how it goes, if we don't get the results we want let's handle that on a separate PR. |
|
I see linter error on the CI, could you also take care of it? These commands can be handy to fix them automatically:
|
|
@mkevins Apologies for the delay, I've only had a chance to take this for a quick spin, but @pinarol basically caught most of the feedback I was going to give (appender spacing, caption styling, etc). I'll comment with more feedback over the weekend, but here is the block blueprint (below) and I'm updating the Zeplin project to help with some of the details.
This is looking really great, it appears we just need some style refinements and we'll be in good shape! Thank you for all of your hard work on it :) |
The way it is rendered,
Thanks for clarifying. For the icons, I notice that they're often rendered as SVG elements local to the given implementation. Is there a plan to extract these at some point? |
|
Hi @iamthomasbishop 👋 Thanks for taking a look at this!
I'm looking forward to your feedback! |
No it is OK that makes total sense.
Not that I know. Maybe we won't need to put icons on empty tiles, this is a design decision. |
|
I had a chance to take this for a spin over the weekend. Awesome work – super exciting to see it functioning and most of the concerns I have are cosmetic, styling issues. The first list are what I would consider "blockers", second list not as much. Blockers
Not blockers:
|
|
Also, catching up on y'all's previous questions for me:
@mkevins That gray background was just a general "filler" for the mocks at that point, but essentially it should be the same background as we have for the equivalent state on image blocks, which I think is either
@pinarol Not critical that we do but, it might be useful. If we do that, let's use the same styling that we're using on the video block (which I've requested that we match on the image block in-between state on this issue – see styling notes there). |
|
One more thing I keep coming back to, which we could improve upon over the web – I should be able to do most things on a single gallery image that I can on an image block. For example, I can't replace an image in the same ways (toolbar button or long-press on image), I can't link a single image manually (not sure if this is something we'll be able to override, but it's frustrating), and I can't edit alt text of any images in a gallery. Maybe if the gallery eventually follows more of a "nesting" hierarchy model these things will become easier/more available, but I'd like to improve them if possible. (Note: I wouldn't consider this stuff a blocker to this specific implementation, just food for thought) |
|
Thanks @iamthomasbishop for this detailed list of improvements 👍 I've added some changes to address some of these points:
I've extracted the usage of
@jbinda or @lukewalczak, do you know if 👆 this
This is partially addressed (the selection color has been updated for light mode). I'll push the style for dark mode soon (blue-30 from color studio). ✔️
This one may be tricky, as I don't believe gradients are supported out of the box in RN. There is a library https://github.com/react-native-community/react-native-linear-gradient, but I didn't see that we are using it. This might require some exploration. I have added some simple transparent dark style as the caption background for now, but this could benefit from some iteration I think. Proposed solution for this iteration: #18155 (comment).
I'm not sure these appender styles will be inherited (block list appender vs. media placeholder appender), but I think we'll be able to address this separately either way.
For this one, I've added another breakpoint for viewport widths < 960 which constrains displayed columns to a maximum of 4. We could change this if necessary. Do you think something like this will address the issue? Available breakpointsconst BREAKPOINTS = {
huge: 1440,
wide: 1280,
large: 960,
medium: 782,
small: 600,
mobile: 480,
};
These colors have been applied as background colors. Screenshots (with prevented image load)
|
|
Thanks for the updates, @mkevins !
Surprising that CSS gradients aren't supported, but considering we'll eventually need to support them for various blocks (button, cover, etc.) maybe now is a good time to do a quick investigatory spike 😄 When you say you're adding a "simple transparent dark style as the caption background for now" do you mean something like this?
If so, that's reasonable and can you make it I have often wondered if we might want to truncate captions so that we can avoid overflow issues like the one Pinar mentioned (and also that input area is super small), but I'm not sure if it needs to be attacked during this specific iteration. Just for giggles I was sketching out ideas for a more scalable text input flow, which would be useful on a variety of cases – that looks something like this:
I think this should be fine, but I'll let you know if it feels off during the next round of testing. |
|
Hi @iamthomasbishop 👋 Pinar's 4th issue from this comment remains to be solved. This is the issue where the caption is too tall if there's a lot of text inside. What font-size should I try with, for starters, which may mitigate this a bit? Any flexibility there? Also, should there be some kind of max height for now? I'll try a few explorations on this and see what I think could work, but any guidance you can provide is greatly appreciated. Thanks! 😄 |
|
Here is what I think we should do regarding the sizing/overflow on captions:
Visual example:
As far as text sizes, here are the specs:
I wouldn't go any smaller than 12px for readability concerns, and the shadow is intended to help with legibility. |
Hi @pinarol 👋 After our discussion regarding the controls styles 👆 I investigated further. To recap: the default separator type styles do not seem to match, so I tried your suggestion by explicitly adding them in gallery diff --git a/packages/block-library/src/gallery/edit.js b/packages/block-library/src/gallery/edit.js
index 3a3aa6c..ff32c50 100644
--- a/packages/block-library/src/gallery/edit.js
+++ b/packages/block-library/src/gallery/edit.js
@@ -299,6 +299,7 @@ class GalleryEdit extends Component {
<PanelBody title={ __( 'Gallery Settings' ) }>
{ images.length > 1 && <RangeControl
label={ __( 'Columns' ) }
+ separatorType="fullWidth"
value={ columns }
onChange={ this.setColumnsNumber }
min={ 1 }
@@ -307,12 +308,14 @@ class GalleryEdit extends Component {
/> }
<ToggleControl
label={ __( 'Crop Images' ) }
+ separatorType="fullWidth"
checked={ !! imageCrop }
onChange={ this.toggleImageCrop }
help={ this.getImageCropHelp }
/>
<SelectControl
label={ __( 'Link To' ) }
+ separatorType="fullWidth"
value={ linkTo }
onChange={ this.setLinkTo }
options={ linkOptions }This seems to address the separator indentation issue, however, removal of the icon still causes problems for Screenshot: Another issue that still needs to be addressed is that the
Are there plans in another PR to address those issues? |
|
Hey @mkevins, sorry for late participation in the discussion.
Didn't see the issue / PR tracking that problem. |
mkevins
force-pushed
the
try/gallery-draft-gallery-image
branch
from
7a38a85 to
b982d1d
Compare
Description
This PR introduces a native
GalleryImagecomponent for the semi-cross-platform Gallery block. The purpose of this component is to render a gallery image on mobile, providing UI controls for the image if the image is selected. It incorporates the designs described here: wordpress-mobile/gutenberg-mobile#1301 (comment).PR Hierarchy
This PR is part of the PR hierarchy described here. This PR can be tested with the aggregate changeset and integration of components within the related PRs by checking out the branch of the "top level" PR: #18265 via the
gutenberg-mobilePR: wordpress-mobile/gutenberg-mobile#1498.Native vs. Web
Much of the code in this component is adapted from the web component counterpart. HTML elements have been replaced with components that work on mobile (i.e.
<div> => <View>,<img> => <Image>, but much of the logic is the same.Since media uploading is handled differently on mobile, this component also incorporates logic to handle events associated with the
MediaUploadProgresscomponent used in its composition.Buttons
Buttons just for GalleryImage (for now)
The web implementation of this component uses
IconButton, which, in turn, usesButton. Our mobile implementation of theButtoncomponent seemingly lacks a way to style the button for the purpose of the gallery image. I have created an implementation ofButtonforGalleryImagehere: #18264. Ideally,Button(orIconButton) from@wordpress/componentscan be general enough to allow re-use from this component. I have opened a separate draft PR for galleryButtonfor the purpose of that discussion.Note: The buttons PR has been merged with this one for review here.
TouchableWithoutFeedback
To make gallery images "touchable", they are wrapped in a
TouchableWithoutFeedback. This state is disabled unless the block is selected. This is similar to "navigating down the hierarchy" with inner blocks.To test
Test this component by checking out the branch of the "top level" PR: #18265 via the
gutenberg-mobilePR: wordpress-mobile/gutenberg-mobile#1498.Image Selection
Tap a gallery image
Expected behavior
Captions
Tap the caption of a selected image. Then tap the image.
Expected behavior
Type rich text into the caption
Expected behavior
Type a very long caption (use newlines / [enter] if you want)
Expected behavior
Known issues:
Movement
Tap the image mover buttons for any selected image
Expected behavior
Removal
Tap the 'X' button on the selected image
Expected behavior
Appender
Tap the appender
Expected behavior
Tap the Media Library option. Select multiple images (multiple selection should be possible).
Expected behavior
Questions
Accessibility
I have not yet tested this implementation via TalkBack nor VoiceOver, and it'd be good to get feedback on the structure introduced here, to provide the best experience possible for users who utilize those tools. Two questions that come to mind:
alttext that is associated with images to enhance / improve the experience?Checklist: