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.

Sign up for GitHub

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

Jump to bottom

Chrome: Allow adding, removing tags from posts #970

Merged
merged 8 commits into from
Jun 6, 2017
Merged

Chrome: Allow adding, removing tags from posts #970

merged 8 commits into from
Jun 6, 2017

Conversation

youknowriad
Copy link
Contributor

This PR make the TagSelector part of #854.

Worth noting:

  • I'm loading the tags in the TagSelector component ( the data discussion not settled yet Our Data Approach #902 )
  • A difference with Calypso here is that the API accepts IDs, which means we need to create the posts before assigning them to the post.

@youknowriad youknowriad added the General Interface Parts of the UI which don't fall neatly under other labels. label Jun 1, 2017
@youknowriad youknowriad self-assigned this Jun 1, 2017
@youknowriad youknowriad requested review from jasmussen and aduth June 1, 2017 09:54
@jasmussen
Copy link
Contributor

I like this a lot! I pushed a little CSS polish to the tag chips.

I noticed when you're creating a new tag, there's a brief moment of the tag being blank. Can we insert a placeholder there? Or simply insert the actual tag and just assume that the database call to save the tag succeeds?

jun-01-2017 12-27-46

youknowriad
youknowriad commented Jun 1, 2017
View reviewed changes
components/form-token-field/style.scss Outdated
@@ -4,8 +4,9 @@
margin: 0;
padding: 0;
background-color: $white;
border-radius: 4px;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess we need tabs here :)

@youknowriad
Copy link
Contributor Author

I noticed when you're creating a new tag, there's a brief moment of the tag being blank. Can we insert a placeholder there? Or simply insert the actual tag and just assume that the database call to save the tag succeeds?

I'll take a look

aduth
aduth reviewed Jun 1, 2017
View reviewed changes
editor/sidebar/post-taxonomies/tags-selector.js Outdated

const DEFAULT_TAGS_QUERY = {
number: 1000,
order_by: 'count',
Copy link
Member

Choose a reason for hiding this comment

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

The correct argument name is orderby:

https://developer.wordpress.org/rest-api/reference/tags/#list-tags

editor/sidebar/post-taxonomies/tags-selector.js Outdated
import { editPost } from '../../actions';

const DEFAULT_TAGS_QUERY = {
number: 1000,
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to make a conscious choice between loading "lots" of tags vs. loading "all" the tags (-1)?

editor/sidebar/post-taxonomies/tags-selector.js

componentWillReceiveProps( newProps ) {
if ( newProps.tags !== this.props.tags ) {
this.updateSelectedTags( newProps.tags );
Copy link
Member

Choose a reason for hiding this comment

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

Why don't we need to updateSelectedTags on initial mount? Wouldn't a post loaded from save already have tags assigned?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but the component won't have the availableTags yet

editor/sidebar/post-taxonomies/tags-selector.js
const tagNamesToIds = ( names, availableTags ) => {
return names
.map( ( tagName ) =>
find( availableTags, ( tag ) => tag.name === tagName ).id
Copy link
Member

Choose a reason for hiding this comment

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

Do we ever worry the find could return undefined here and throw an error on property access?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not at this moment, because we ensure all the tagNames are loaded before calling this.

editor/sidebar/post-taxonomies/tags-selector.js
}
const createTag = ( tagName ) => new wp.api.models.Tag( { name: tagName } ).save();
Promise
.all( newTagNames.map( createTag ) )
Copy link
Member

Choose a reason for hiding this comment

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

Do we actually need or want to save these tags when they're added? AFAIK the post endpoint is happy to create these for us when the post is saved if they don't yet exist, and may actually be the preferred behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried but failed, which shape should these tags have to do this? The endpoint was giving me an error like tags should be integers (which means ids for me)

Copy link
Member

Choose a reason for hiding this comment

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

Dug around a bit and it appears intentional the endpoint only accepts IDs:

https://github.com/WordPress/WordPress/blob/1d34043/wp-includes/rest-api/endpoints/class-wp-rest-posts-controller.php#L2038

editor/sidebar/post-taxonomies/tags-selector.js Outdated
suggestions={ suggestions }
onChange={ this.onTokensChange }
/>
{ loading && <Spinner /> }
Copy link
Member

Choose a reason for hiding this comment

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

How do you feel about a disabled token field instead of a spinner? Is that any better or worse for perceived reactivity?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll try

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tested and approved :)

@nylen
Copy link
Member

nylen commented Jun 1, 2017

@youknowriad for the slow TokenField tests I think a reasonable approach is #982 and then find a way to run these regularly outside of Travis.

@youknowriad youknowriad force-pushed the update/tag-selector branch from cf2da64 to 45770d4 Compare June 2, 2017 13:51
@youknowriad youknowriad force-pushed the update/tag-selector branch from 45770d4 to 0905da4 Compare June 5, 2017 08:16
@youknowriad
Copy link
Contributor Author

I'll be merging this shortly if no objections?

@youknowriad youknowriad merged commit c3bb0bb into master Jun 6, 2017
@youknowriad youknowriad deleted the update/tag-selector branch June 6, 2017 12:24
@youknowriad youknowriad mentioned this pull request Jun 20, 2017
Closed
@npneeraj npneeraj mentioned this pull request Jul 7, 2023
Open
@soloinovator soloinovator mentioned this pull request Jul 8, 2023
Open
@Verdinjoshua26 Verdinjoshua26 mentioned this pull request Jul 8, 2023
Open
@yassine4999 yassine4999 mentioned this pull request Jul 8, 2023
Open
@MaxMood96 MaxMood96 mentioned this pull request Jul 8, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aduth aduth aduth left review comments

@jasmussen jasmussen Awaiting requested review from jasmussen

Assignees

@youknowriad youknowriad

Labels
General Interface Parts of the UI which don't fall neatly under other labels.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@youknowriad @jasmussen @nylen @aduth