Have a image placeholder while image size is being calculated.

[SergioEstevao]

Description

Adds an image placeholder while ImageSize is calculating the dimensions of the image.

Refs: wordpress-mobile/gutenberg-mobile#451

[hypest]

while ImageSize is calculating the dimensions

👋 @SergioEstevao , why do we care about the specific time the image size is being computed? I assume that that's taken as a proxy that the image is not yet downloaded but, not sure. I think I might be missing some context here and it's not intuitive to find out.

Can you expand the PR description with some info on why do we do this solution? Ideally, adding some inline comments in the code can also help.

[hypest]

Returning early is fine but can be tricky understanding the code when skimming through. I think the code outside is rather trivial and small and I'd suggest we put it in an else block to help with readability. WDYT?

[hypest]

The lint rules in GB complain about that :).

Ah, there's no-else-return in place, right!

[hypest]

WDYT about moving the style definition in a style file? There's already the ./styles.native.scss we can use.

[hypest]

@SergioEstevao , can you elaborate on the rationale behind having the placeholder logic be inside the ImageSize component instead of the parent? Seems to me that we're making this component more complex if besides image sizes, we also alter the rendered children here. 🤔

[SergioEstevao]

So one thing that makes we see the white screen, is that before we show anything image size need to download the image, find its size, calculate the optimal size to display and then display the image. This code was already in place not changed by me. What I did is that while that action is happening we show a placeholder image.

…

Sent from my iPhone

On 11 Feb 2019, at 08:34, Stefanos Togoulidis ***@***.***> wrote: while ImageSize is calculating the dimensions 👋 @SergioEstevao , why do we care about the specific time the image size is being computed? I assume that that's takes as a proxy that the image is not yet downloaded but, not sure. I think I might be missing some context here and it's not intuitive to find out. Can you expand the PR description with some info on why do we do this solution? Ideally, adding some inline comments in the code can also help. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[SergioEstevao]

The lint rules in GB complain about that :).

…

Sent from my iPhone

On 11 Feb 2019, at 08:39, Stefanos Togoulidis ***@***.***> wrote: @hypest commented on this pull request. In packages/block-library/src/image/image-size.native.js: > @@ -76,6 +77,14 @@ class ImageSize extends Component { imageWidthWithinContainer: this.state.width, imageHeightWithinContainer: this.state.height, }; + if ( this.state.width === undefined ) { + const width = 300; + return ( + <View onLayout={ this.onLayout } style={ { flex: 1, flexDirection: 'row', justifyContent: 'center' } }> + <Dashicon icon={ 'format-image' } size={ width } /> + </View> + ); + } Returning early is fine but can be tricky understanding the code when skimming through. I think the code outside is rather trivial and small and I'd suggest we put it in an else block to help with readability. WDYT? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[hypest]

So one thing that makes we see the white screen, is that before we show anything image size need to download the image, find its size, calculate the optimal size to display and then display the image. This code was already in place not changed by me. What I did is that while that action is happening we show a placeholder image.

OK, thanks for expanding @SergioEstevao. Then I think the only open question for me is whether we are forced to render the placeholder here in the ImageSize component or that can be done in its children here: https://github.com/WordPress/gutenberg/blob/master/packages/block-library/src/image/edit.native.js#L288-L322

[SergioEstevao]

@hypest updated the code according to your commentaries. Can you please give it another look?

[hypest]

LGTM, thanks for the changes @SergioEstevao !