-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Chrome: Adding the post category selector #1159
Conversation
Just a slight observation and not exactly relevant to this pull request, but it would make sense to have the title as "Tags & Categories" seeing as the Categories are below the Tags. I feel Categories are used more than Tags these days, so I would just move the Categories above the Tags, personally. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice one. This works quite well for me in my testing.
* External dependencies | ||
*/ | ||
import { connect } from 'react-redux'; | ||
import { unescape, without, groupBy } from 'lodash'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I always worry about unescape
since it's available as a function the global scope (shadowing):
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/unescape
Also unique syntax highlighting in my editor.
unescape as unescapeString
(or alternative) is an option, though admittedly ugly.
Maybe fine to accept the shadowing, especially since #1008 will flag if usage doesn't align with an import
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I don't care that much about the name. I'm using unescapeString
*/ | ||
import { __ } from 'i18n'; | ||
import { Component } from 'element'; | ||
import { getEditedPostAttribute } from '../../selectors'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are "Internal" dependencies.
import { editPost } from '../../actions'; | ||
|
||
const DEFAULT_CATEGORIES_QUERY = { | ||
number: -1, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, I think this is meant to be per_page
https://developer.wordpress.org/rest-api/reference/categories/#list-categorys
Same goes for tags which was already merged:
|
||
return ( | ||
<div className="editor-post-taxonomies__categories-selector"> | ||
<strong className="editor-post-taxonomies__categories-selector-title">{ __( 'Categories' ) }</strong> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we use a proper heading element here instead of strong
? Similarly I might have expected the panel toggle headings to be heading elements.
…ctors title - Avoid shadowing the escape function - Fix the taxonomies query
Thanks for the reviews @aduth @paulwilde Feedback addressed |
Really appreciate the introduction of headings, thanks ❤️ |
Closes #854
This PR adds the category selector to the post settings panel