Blocks: Fix image block error with .tiff images, refactor using withAPIData #2840
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2840 +/- ##
==========================================
+ Coverage 33.74% 33.79% +0.04%
==========================================
Files 191 191
Lines 5695 5688 -7
Branches 997 995 -2
==========================================
Hits 1922 1922
+ Misses 3192 3187 -5
+ Partials 581 579 -2
Continue to review full report at Codecov.
|
|
Which browser are you using for testing, it's not working on Chrome. I guess it's the preview issue? |
| @@ -73,31 +70,20 @@ class ImageBlock extends Component { | |||
| this.props.setAttributes( { ...extraUpdatedAttributes, align: nextAlign } ); | |||
| } | |||
|
|
|||
| fetchMedia( id ) { | |||
There was a problem hiding this comment.
HOCs remove so much code duplication 👍
| } | ||
|
|
||
| return { | ||
| image: `/wp/v2/media/${ id }`, |
There was a problem hiding this comment.
It's surprising that you need to combine the endpoint's url here, but otherwise it's very nice 👍
There was a problem hiding this comment.
It's surprising that you need to combine the endpoint's url here
Could you elaborate on what you mean by this? I'm not sure if the surprise is something I should be worried about 😄
There was a problem hiding this comment.
I would expect to see API url creation hidden in the HOC. So in this place we could have the following instead:
return [
id && { type: 'image', params: { id } }
];withAPIData could filter out undefined elements to make this more conceise here.
At the moment we have the following code in a few places:
const applyWithAPIData = withAPIData( () => {
return {
user: '/wp/v2/users/me?context=edit',
};
} );which would simplify to:
const applyWithAPIData = withAPIData( () => [ { type: 'user' } ] );withAPIData would have all mappings stored internally:
user => /wp/v2/users/me?context=edit
I might be missing something obvious here, but I'm happy to discuss if you think it would be an improvement. It's good how it is because those urls won't rather change in the future :)
There was a problem hiding this comment.
The issue I'd see is type wouldn't always be so simple. This could potentially be used for custom endpoints, which can be defined as whichever route structure the developer chooses (even /this/is/an/obnoxious/route/pattern). I noted some downsides here in the original text of #1974, including how this binds us to the REST API, but doing so in a way that is easy for developers to use (don't need to understand mappings if they already know the routes), isolated, generic.
|
Code changes look good, I haven't tested though. |
What specifically is not working? On master it currently errors the whole block (captured by
|
Fixes #2535
This pull request seeks to refactor the image block to use the
withAPIDatahigher-order component for image data request, and in doing so, resolve an error described at #2535 related to.tifffile uploads. Such files should no longer cause the block to error, but they do not preview well in all browsers. We may separately want to explore better previews for images which cannot be displayed in the current browser.Testing instructions:
Repeat steps to reproduce from #2535, ensuring that no errors occur when assigning a
.tiffimage to an image block.Example: https://cldup.com/h4c6dcv9pg.tiff
Verify that there are no regressions in existing image block behavior, particularly: