Mobile: update image size picker #31963
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Size Change: 0 B Total Size: 1.05 MB ℹ️ View Unchanged
|
2 tasks
|
@enejb Looks good to me! Stoked we're changing Cycle into Select; makes a lot more sense! |
geriux
reviewed
May 20, 2021
Comment on lines
118
to
125
| filter( this.props.imageSizes, ( { slug } ) => | ||
| get( this.props.image, [ | ||
| 'media_details', | ||
| 'sizes', | ||
| slug, | ||
| 'source_url', | ||
| ] ) | ||
| ), |
There was a problem hiding this comment.
Hey @enejb 👋 I was testing this PR but I couldn't get the size options to show up. If I remove this filter they show up correctly. Does it work for you with the current changes?
There was a problem hiding this comment.
I was running into the same issue.
I think it has to do if we know about the different sizes that are available.
This filter was taken from
Currently, if you remove the filter and then try to set the size of the image that doesn't exist ( some images do not have all the different sizes) that change will not work as expected anyways.
See
That being said. Maybe there is something mobile-specific that I don't quite understand when it comes to images and when we have the information about the different image sizes.
cc: @mkevins Do you understand why sometimes the images do not have the different sizes available?
There was a problem hiding this comment.
Do you understand why sometimes the images do not have the different sizes available?
I'm not entirely sure, but I imagine that it's possible from several possibilities. One could be that the available sizes are changed at some point in the site, or possibly that the uploaded image isn't converted to the various sizes from ImageMagick. Another possibility that comes to mind, if you encountered this after uploading, is that the images sizes were not updated in the component after the upload completed?
I found this comment, which might be worth investigating: https://github.com/WordPress/gutenberg/pull/28238/files#r570481358 since the sizes come from the store, but after that performance refactor are only set in the constructor. 🤔
There was a problem hiding this comment.
As a side note, I wonder if we can remove the lodash get here, since optional chaining is available. Wdyt?
7d87c9e to
fafe93e
Compare
|
cc @geriux can you take a look at this PR again when you have a moment? |
Before we we calculating the sizeOptions in the constructor which didn't happen often enough. And cased issue when you first inserted the image.
9587f5b to
37be821
Compare
|
Thanks for the review @geriux. It seems that I somehow must have lost the icon in a rebase. See cc: @iamthomasbishop can you confirm that it is the correct icon? |
geriux
approved these changes
Jul 6, 2021
sarayourfriend
pushed a commit
that referenced
this pull request
Jul 15, 2021
* Update Bottom Sheet Select Control to accept a label icon * Update the Image block to use Bottom Sheet Select Control * Minor code fixes * Fix code style * Make sure that we always recalculate the possible sizeOptions. Before we calculating the sizeOptions in the constructor which didn't happen often enough. And caused an issue when you first inserted the image. * Remove lodash find, isEmpty, map and filter * Double check that we are dealing with arrays before proceeding. * Use the same icon - fullscreen
12 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR updates the image size picker to use Bottom Sheet Control instead of the current cycle control
This helps the user pick the right size of image that they want to use. Without cycling though all of the image size and downloading all the different images.
It also fixes an issue where we are only making the image sizes that are selectable available to be selected.
How has this been tested?
Did it work as expected?
Does it work the same way on the web. If you upload a smaller image then you should only see some of the sizes and not all.
Screenshots
Before
After
Types of changes
UI update
Checklist:
*.native.jsfiles for terms that need renaming or removal).