Skip to content
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

Components: Display post type adjacent PostSelector option title #3264

Merged
merged 6 commits into from
Feb 18, 2016

Conversation

aduth
Copy link
Contributor

@aduth aduth commented Feb 11, 2016

Related: #3197, #3251

This pull request seeks to update the <PostSelector /> component to display an option's type if the selector query is set to request any post type.

Before After
Before After

Testing instructions:

Verify that the post type is shown for <PostSelector /> options, but only if the selector is set to retrieve any post type for the site. Specifically, you should find that the Calypso page editor instance of <PostSelector /> should not display the type (since it only queries pages), whereas the App Components demo should display the type.

Page editor:

  1. Navigate to the Calypso page editor
  2. Select a site, if prompted
  3. Expand the Page Options sidebar accordion
  4. Note that the type is not shown adjacent the option title

App Components demo:

  1. Navigate to the Calypso DevDocs App Components page
  2. Note that types are not shown adjacent the option title in the Post Selector demo

@aduth aduth added Posts [Feature] Post/Page Editor The editor for editing posts and pages. [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. Components labels Feb 11, 2016
@aduth aduth self-assigned this Feb 11, 2016
@@ -131,6 +134,7 @@ const PostSelectorPosts = React.createClass( {
const checked = this.props.selected === item.ID;
const inputType = this.props.multiple ? 'checkbox' : 'radio';
const domId = camelCase( this.props.analyticsPrefix ) + '-option-' + itemId;
const postType = get( this.props.postTypes, [ item.type, 'labels', 'singular_name' ] );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These labels are translated API-side correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These labels are translated API-side correct?

Actually not, now that you mention it. Localization is opt-in, so the legacy store is not localized either. Furthermore, there was a bug in the REST API which was causing default post types (post, page, etc) to not respect the locale parameter, which has been fixed in the past day (r131176-wpcom). #3348 seeks to address the need for broad localization support for all wpcom.js usage, at which point we can look to use it in the state/post-types/actions.js action creator for requesting post types.

@timmyc
Copy link
Contributor

timmyc commented Feb 16, 2016

I see that the labels are only shown when no specific post-type is being queried - which is cool - what do you think about adding a bool prop to the PostSelector that can also force the labels to be hidden?

One other thing that came to mind while reviewing this is one of @folletto 's designs for the selector used gridicons to the left of the post title for the same purpose - so I figured it would be worth mentioning here.

This is working out well for me in my testing, but it looks like we could run into some issues with long post titles with the addition of this label:

wordpress_com

Additionally by using a text label here, we need to keep in mind the translated versions of the labels might make this more of an issue too.

Confirmed all looks the same in the page parent selector.

@timmyc timmyc added [Status] Needs Author Reply and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Feb 16, 2016
@aduth
Copy link
Contributor Author

aduth commented Feb 17, 2016

Thanks for the review @timmyc !

I see that the labels are only shown when no specific post-type is being queried - which is cool - what do you think about adding a bool prop to the PostSelector that can also force the labels to be hidden?

Yeah, that sounds like a good idea. I've added an optional prop showTypeLabels in 87eb14c. If omitted, the default behavior (only any requests) takes effect, otherwise it respects the value passed. This could be used to enable the labels in non-any cases or disable them when requesting any.

One other thing that came to mind while reviewing this is one of @folletto 's designs for the selector used gridicons to the left of the post title for the same purpose

I can't recall the original design you're referencing. As implemented here, this follows very similarly to the wp-admin interface. Personally, I'd argue that the text label is more likely to better convey its meaning. Open to differing opinions though!

This is working out well for me in my testing, but it looks like we could run into some issues with long post titles with the addition of this label:

This should be fixed in c0b4f00, using display: flex and justify-content: space-between.

Additionally by using a text label here, we need to keep in mind the translated versions of the labels might make this more of an issue too.

Agree, we should definitely plan to have this fixed before deploy any widespread usage of this component. However, since the only live usage is in the parent page selector, we could probably move forward with the changes here parallel to efforts described in #3264 (comment).

@aduth aduth added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] Needs Author Reply labels Feb 17, 2016
@timmyc
Copy link
Contributor

timmyc commented Feb 17, 2016

Changes look great - no issues with post type labels and long page titles now 👍

Thanks for the prop too, that is a nice addition.

🚢 🔑

@timmyc timmyc added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Feb 17, 2016
@aduth aduth force-pushed the update/post-selector-type branch from 87eb14c to 0b67369 Compare February 18, 2016 15:39
@aduth aduth force-pushed the update/post-selector-type branch from 0b67369 to 4470f1b Compare February 18, 2016 17:10
aduth added a commit that referenced this pull request Feb 18, 2016
Components: Display post type adjacent PostSelector option title
@aduth aduth merged commit c031164 into master Feb 18, 2016
@aduth aduth deleted the update/post-selector-type branch February 18, 2016 20:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Components [Feature] Post/Page Editor The editor for editing posts and pages. Posts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants