Chrome: Allow adding, removing tags from posts #970
Conversation
youknowriad
added
the
General Interface
|
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?
|
| @@ -4,8 +4,9 @@ | |||
| margin: 0; | |||
| padding: 0; | |||
| background-color: $white; | |||
| border-radius: 4px; | |||
There was a problem hiding this comment.
I guess we need tabs here :)
I'll take a look |
|
|
||
| const DEFAULT_TAGS_QUERY = { | ||
| number: 1000, | ||
| order_by: 'count', |
There was a problem hiding this comment.
The correct argument name is orderby:
https://developer.wordpress.org/rest-api/reference/tags/#list-tags
| import { editPost } from '../../actions'; | ||
|
|
||
| const DEFAULT_TAGS_QUERY = { | ||
| number: 1000, |
There was a problem hiding this comment.
Do we want to make a conscious choice between loading "lots" of tags vs. loading "all" the tags (-1)?
|
|
||
| componentWillReceiveProps( newProps ) { | ||
| if ( newProps.tags !== this.props.tags ) { | ||
| this.updateSelectedTags( newProps.tags ); |
There was a problem hiding this comment.
Why don't we need to updateSelectedTags on initial mount? Wouldn't a post loaded from save already have tags assigned?
There was a problem hiding this comment.
but the component won't have the availableTags yet
| const tagNamesToIds = ( names, availableTags ) => { | ||
| return names | ||
| .map( ( tagName ) => | ||
| find( availableTags, ( tag ) => tag.name === tagName ).id |
There was a problem hiding this comment.
Do we ever worry the find could return undefined here and throw an error on property access?
There was a problem hiding this comment.
Not at this moment, because we ensure all the tagNames are loaded before calling this.
| } | ||
| const createTag = ( tagName ) => new wp.api.models.Tag( { name: tagName } ).save(); | ||
| Promise | ||
| .all( newTagNames.map( createTag ) ) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
Dug around a bit and it appears intentional the endpoint only accepts IDs:
| suggestions={ suggestions } | ||
| onChange={ this.onTokensChange } | ||
| /> | ||
| { loading && <Spinner /> } |
There was a problem hiding this comment.
How do you feel about a disabled token field instead of a spinner? Is that any better or worse for perceived reactivity?
There was a problem hiding this comment.
Tested and approved :)
|
@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
force-pushed
the
update/tag-selector
branch
from
cf2da64
to
45770d4
Compare
This reverts commit d384b451149a6d971bece019591fc6c655a83572.
youknowriad
force-pushed
the
update/tag-selector
branch
from
45770d4
to
0905da4
Compare
|
I'll be merging this shortly if no objections? |
This PR make the TagSelector part of #854.
Worth noting: