New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Blocks: Fix image block error with .tiff images, refactor using withAPIData #2840

Merged
merged 1 commit into from Oct 3, 2017

Conversation

Projects
None yet
3 participants
@aduth
Member

aduth commented Sep 30, 2017

Fixes #2535

This pull request seeks to refactor the image block to use the withAPIData higher-order component for image data request, and in doing so, resolve an error described at #2535 related to .tiff file 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 .tiff image to an image block.

Example: https://cldup.com/h4c6dcv9pg.tiff

Verify that there are no regressions in existing image block behavior, particularly:

  • Inserting a new image block and assigning an image
  • Selecting a thumbnail size
  • Dragging to resize an image

@aduth aduth added the Blocks label Sep 30, 2017

@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Sep 30, 2017

Codecov Report

Merging #2840 into master will increase coverage by 0.04%.
The diff coverage is 0%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
blocks/library/image/block.js 0% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a596ceb...799d17b. Read the comment docs.

codecov bot commented Sep 30, 2017

Codecov Report

Merging #2840 into master will increase coverage by 0.04%.
The diff coverage is 0%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
blocks/library/image/block.js 0% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a596ceb...799d17b. Read the comment docs.

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Oct 2, 2017

Contributor

Which browser are you using for testing, it's not working on Chrome. I guess it's the preview issue?

Contributor

youknowriad commented Oct 2, 2017

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 ) {

This comment has been minimized.

@gziolo

gziolo Oct 2, 2017

Member

HOCs remove so much code duplication 👍

@gziolo

gziolo Oct 2, 2017

Member

HOCs remove so much code duplication 👍

}
return {
image: `/wp/v2/media/${ id }`,

This comment has been minimized.

@gziolo

gziolo Oct 2, 2017

Member

It's surprising that you need to combine the endpoint's url here, but otherwise it's very nice 👍

@gziolo

gziolo Oct 2, 2017

Member

It's surprising that you need to combine the endpoint's url here, but otherwise it's very nice 👍

This comment has been minimized.

@aduth

aduth Oct 2, 2017

Member

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 😄

@aduth

aduth Oct 2, 2017

Member

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 😄

This comment has been minimized.

@gziolo

gziolo Oct 3, 2017

Member

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 :)

@gziolo

gziolo Oct 3, 2017

Member

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 :)

This comment has been minimized.

@aduth

aduth Oct 3, 2017

Member

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.

@aduth

aduth Oct 3, 2017

Member

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.

@gziolo

This comment has been minimized.

Show comment
Hide comment
@gziolo

gziolo Oct 2, 2017

Member

Code changes look good, I haven't tested though.

Member

gziolo commented Oct 2, 2017

Code changes look good, I haven't tested though.

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Oct 2, 2017

Member

Which browser are you using for testing, it's not working on Chrome. I guess it's the preview issue?

What specifically is not working? On master it currently errors the whole block (captured by componentDidCatch). That is fixed here, but yes, it does not preview very well.

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.

Member

aduth commented Oct 2, 2017

Which browser are you using for testing, it's not working on Chrome. I guess it's the preview issue?

What specifically is not working? On master it currently errors the whole block (captured by componentDidCatch). That is fixed here, but yes, it does not preview very well.

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.

@youknowriad

No block error anymore and still works great for the other formats

@aduth aduth merged commit ad4450c into master Oct 3, 2017

3 checks passed

codecov/project 33.79% (+0.04%) compared to a596ceb
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@aduth aduth deleted the fix/2535-tiff-image branch Oct 3, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment