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

Fixed bad grammar in featured image upload. #3396

Merged
merged 2 commits into from Nov 27, 2017

Conversation

Projects
None yet
2 participants
@jorgefilipecosta
Member

jorgefilipecosta commented Nov 8, 2017

This PR aims to fix #3394. An optional title parameter was added to the media upload button so we can easily change the title of the modal area in similar situations.

Testing

  1. Add a featured image.
  2. Verify that the title of the image selection area is "Select a Featured Image"
  3. Add the featured image.
  4. Add an image or gallery block verify their titles stay the same as before.

Screenshots (jpeg or gifs if applicable):

screen shot 2017-11-08 at 21 22 46

@aduth

Ideally we'd be using strings from the post type definition, which can be customized for CPTs and filtered:

https://github.com/WordPress/WordPress/blob/0a81bcf4fffef460f22fda3fe5ccdf0376d1e3d9/wp-includes/post.php#L1403

Show outdated Hide outdated blocks/media-upload-button/index.js Outdated
@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Nov 9, 2017

Member

Ideally we'd be using strings from the post type definition, which can be customized for CPTs and filtered:

Noting that these are made available on a labels property from the /wp/v2/types/<type> endpoint when ?context=edit is passed.

Member

aduth commented Nov 9, 2017

Ideally we'd be using strings from the post type definition, which can be customized for CPTs and filtered:

Noting that these are made available on a labels property from the /wp/v2/types/<type> endpoint when ?context=edit is passed.

@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Nov 20, 2017

Codecov Report

Merging #3396 into master will increase coverage by 0.02%.
The diff coverage is 75%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3396      +/-   ##
==========================================
+ Coverage    37.3%   37.33%   +0.02%     
==========================================
  Files         277      277              
  Lines        6690     6691       +1     
  Branches     1214     1213       -1     
==========================================
+ Hits         2496     2498       +2     
  Misses       3536     3536              
+ Partials      658      657       -1
Impacted Files Coverage Δ
blocks/media-upload-button/index.js 4.25% <ø> (ø) ⬆️
editor/components/post-featured-image/index.js 40% <75%> (+17.77%) ⬆️

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 66ac489...af216c5. Read the comment docs.

codecov bot commented Nov 20, 2017

Codecov Report

Merging #3396 into master will increase coverage by 0.02%.
The diff coverage is 75%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3396      +/-   ##
==========================================
+ Coverage    37.3%   37.33%   +0.02%     
==========================================
  Files         277      277              
  Lines        6690     6691       +1     
  Branches     1214     1213       -1     
==========================================
+ Hits         2496     2498       +2     
  Misses       3536     3536              
+ Partials      658      657       -1
Impacted Files Coverage Δ
blocks/media-upload-button/index.js 4.25% <ø> (ø) ⬆️
editor/components/post-featured-image/index.js 40% <75%> (+17.77%) ⬆️

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 66ac489...af216c5. Read the comment docs.

@jorgefilipecosta

This comment has been minimized.

Show comment
Hide comment
@jorgefilipecosta

jorgefilipecosta Nov 20, 2017

Member

Hi @aduth, thank you for your suggestions. I end up using labels form the rest API. And I corrected the grammar of the default title in the image choosing modal.

Member

jorgefilipecosta commented Nov 20, 2017

Hi @aduth, thank you for your suggestions. I end up using labels form the rest API. And I corrected the grammar of the default title in the image choosing modal.

Show outdated Hide outdated editor/components/post-featured-image/index.js Outdated
return {
media: `/wp/v2/media/${ featuredImageId }`,
media: featuredImageId ? `/wp/v2/media/${ featuredImageId }` : undefined,
postType: postTypeName ? `/wp/v2/types/${ postTypeName }?context=edit` : undefined,

This comment has been minimized.

@aduth

aduth Nov 27, 2017

Member

Unfortunately you cannot rely on the post type slug, since a plugin author can define a different rest_base for their custom post types:

https://developer.wordpress.org/rest-api/extending-the-rest-api/adding-rest-api-support-for-custom-content-types/

You can see this in core behavior, where /wp/v2/types/attachment is incorrect, since the rest_base for the attachment post type has been assigned as media.

This is why the type helper from withAPIData second argument should be used.

@aduth

aduth Nov 27, 2017

Member

Unfortunately you cannot rely on the post type slug, since a plugin author can define a different rest_base for their custom post types:

https://developer.wordpress.org/rest-api/extending-the-rest-api/adding-rest-api-support-for-custom-content-types/

You can see this in core behavior, where /wp/v2/types/attachment is incorrect, since the rest_base for the attachment post type has been assigned as media.

This is why the type helper from withAPIData second argument should be used.

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Nov 27, 2017

Member

Hi @aduth, thank you for having a look into this. I used the post type slug because it looks like this specific endpoint relies on it instead of the type.
E.g .../wp-json/wp/v2/types/attachment is a valid request, while .../wp-json/wp/v2/types/media is not a valid request.
The some for posts the type is 'posts' and if we use it we create an invalid request, while we use the slug the request is valid.

@jorgefilipecosta

jorgefilipecosta Nov 27, 2017

Member

Hi @aduth, thank you for having a look into this. I used the post type slug because it looks like this specific endpoint relies on it instead of the type.
E.g .../wp-json/wp/v2/types/attachment is a valid request, while .../wp-json/wp/v2/types/media is not a valid request.
The some for posts the type is 'posts' and if we use it we create an invalid request, while we use the slug the request is valid.

This comment has been minimized.

@aduth

aduth Nov 27, 2017

Member

Aha! That makes sense, sorry for the quick instinctual reaction 😄

@aduth

aduth Nov 27, 2017

Member

Aha! That makes sense, sorry for the quick instinctual reaction 😄

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Nov 27, 2017

Member

No problem at all, at the first I also tried to use post type and then I found it really really strange when it was not working as I expected 😄

@jorgefilipecosta

jorgefilipecosta Nov 27, 2017

Member

No problem at all, at the first I also tried to use post type and then I found it really really strange when it was not working as I expected 😄

return {
media: `/wp/v2/media/${ featuredImageId }`,
media: featuredImageId ? `/wp/v2/media/${ featuredImageId }` : undefined,
postType: postTypeName ? `/wp/v2/types/${ postTypeName }?context=edit` : undefined,

This comment has been minimized.

@aduth

aduth Nov 27, 2017

Member

Aha! That makes sense, sorry for the quick instinctual reaction 😄

@aduth

aduth Nov 27, 2017

Member

Aha! That makes sense, sorry for the quick instinctual reaction 😄

Show outdated Hide outdated editor/components/post-featured-image/index.js Outdated
@aduth

aduth approved these changes Nov 27, 2017

👍

import { editPost } from '../../actions';
function PostFeaturedImage( { featuredImageId, onUpdateImage, onRemoveImage, media } ) {
//used when labels from post tyoe were not yet loaded or when they are not present.
const DEFAULT_SET_FEATURE_IMAGE_LABEL = __( 'Set featured image' );

This comment has been minimized.

@aduth

aduth Nov 27, 2017

Member

Minor: We may not want to assume that __ is a trivial operation, if we could avoid calling it as a side-effect of the module being imported (i.e. before we even know that we'll render it). Further, if we ever wanted to support changing locale in the current page session, these would always show the original value (vs. a forceful re-render of the whole app calling to get the latest value).

On second thought, if we do consider __ as non-trivial, we also have a similar problem that re-renders may occur frequently. Maybe we want to optimize it to be as trivial as possible, in which case it's not as big an issue to assign here.

@aduth

aduth Nov 27, 2017

Member

Minor: We may not want to assume that __ is a trivial operation, if we could avoid calling it as a side-effect of the module being imported (i.e. before we even know that we'll render it). Further, if we ever wanted to support changing locale in the current page session, these would always show the original value (vs. a forceful re-render of the whole app calling to get the latest value).

On second thought, if we do consider __ as non-trivial, we also have a similar problem that re-renders may occur frequently. Maybe we want to optimize it to be as trivial as possible, in which case it's not as big an issue to assign here.

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Nov 27, 2017

Member

I opted for this version because I saw other similar usages e.g: TABLE_CONTROLS in table-block.js, or ALIGNMENT_CONTROLS in alignment-toolbar. But your point: "Further, if we ever wanted to support changing locale in the current page session", is really important not necessarily in this case (because we will get the labels from the API), but in other cases, it may be problematic. I think in the interest of allowing local changes without full refresh we may move __ calls into the render. But even that would be problematic because there is no guarantee the refresh would happen (unless another prop changes), but our odds of updating the text would be better.

@jorgefilipecosta

jorgefilipecosta Nov 27, 2017

Member

I opted for this version because I saw other similar usages e.g: TABLE_CONTROLS in table-block.js, or ALIGNMENT_CONTROLS in alignment-toolbar. But your point: "Further, if we ever wanted to support changing locale in the current page session", is really important not necessarily in this case (because we will get the labels from the API), but in other cases, it may be problematic. I think in the interest of allowing local changes without full refresh we may move __ calls into the render. But even that would be problematic because there is no guarantee the refresh would happen (unless another prop changes), but our odds of updating the text would be better.

jorgefilipecosta added some commits Nov 8, 2017

Post featured image: Fixed bad grammar in featured image upload.
An optional title parameter as added to media upload button.

@jorgefilipecosta jorgefilipecosta merged commit 6bcc8fd into master Nov 27, 2017

3 checks passed

codecov/project 37.33% (+0.02%) compared to 66ac489
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@jorgefilipecosta jorgefilipecosta deleted the fix/bad-grammar-in-featured-image-upload branch Nov 27, 2017

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