-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Parent Page Selector: use an accessible autocomplete/search component #16666
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,44 +1,195 @@ | ||
/** | ||
* External dependencies | ||
*/ | ||
import { get } from 'lodash'; | ||
import { get, debounce } from 'lodash'; | ||
import Autocomplete from 'accessible-autocomplete/react'; | ||
|
||
/** | ||
* WordPress dependencies | ||
*/ | ||
import { __ } from '@wordpress/i18n'; | ||
import { sprintf, __, _n } from '@wordpress/i18n'; | ||
import { TreeSelect } from '@wordpress/components'; | ||
import { compose } from '@wordpress/compose'; | ||
import { withSelect, withDispatch } from '@wordpress/data'; | ||
import { Component } from '@wordpress/element'; | ||
import apiFetch from '@wordpress/api-fetch'; | ||
|
||
/** | ||
* Internal dependencies | ||
*/ | ||
import { buildTermsTree } from '../../utils/terms'; | ||
|
||
export function PageAttributesParent( { parent, postType, items, onUpdateParent } ) { | ||
const isHierarchical = get( postType, [ 'hierarchical' ], false ); | ||
const parentPageLabel = get( postType, [ 'labels', 'parent_item_colon' ] ); | ||
const pageItems = items || []; | ||
if ( ! isHierarchical || ! parentPageLabel || ! pageItems.length ) { | ||
return null; | ||
export class PageAttributesParent extends Component { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we extract a general autocomplete/combobox component for the components package out of this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, my goal is extract this as a reusable component. |
||
constructor() { | ||
super( ...arguments ); | ||
this.searchCache = []; | ||
this.getCurrentParentFromAPI = this.getCurrentParentFromAPI.bind( this ); | ||
this.handleSelection = this.handleSelection.bind( this ); | ||
this.suggestPage = this.suggestPage.bind( this ); | ||
|
||
this.requestResults = debounce( ( query, populateResults ) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, why is this function inlined here while all the other ones are defined outside of the constructor and then bound here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. probably for ease of adding the debounce? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
const payload = '?search=' + encodeURIComponent( query ); | ||
apiFetch( { path: `/wp/v2/pages${ payload }` } ).then( ( results ) => { | ||
populateResults( this.resolveResults( results ) ); | ||
this.searchCache[ query ] = results; | ||
} ); | ||
}, 300 ); | ||
this.state = { | ||
parentPost: false, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's more semantic to use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
}; | ||
} | ||
|
||
/** | ||
* Retrieve the parent page by id. | ||
* | ||
* @param {number} parentId The id of the parent to fetch. | ||
*/ | ||
async getCurrentParentFromAPI( parentId ) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok, I can change - good suggestion. |
||
if ( ! parentId ) { | ||
return ''; | ||
} | ||
const parentPost = await apiFetch( { path: `/wp/v2/pages/${ parentId }` } ); | ||
this.setState( { | ||
parentPost, | ||
} ); | ||
} | ||
|
||
/** | ||
* Resolve the results for display. | ||
* | ||
* @param {Array} results The array of pages that matched the search. | ||
* | ||
* @return {Array} an array of strings ready for displaying. | ||
*/ | ||
resolveResults( results ) { | ||
return results.map( ( item ) => item.title.rendered ? `${ item.title.rendered } (#${ item.id })` : `${ __( 'no title' ) } (#${ item.id })` ); | ||
} | ||
|
||
const pagesTree = buildTermsTree( pageItems.map( ( item ) => ( { | ||
id: item.id, | ||
parent: item.parent, | ||
name: item.title.raw ? item.title.raw : `#${ item.id } (${ __( 'no title' ) })`, | ||
} ) ) ); | ||
return ( | ||
<TreeSelect | ||
className="editor-page-attributes__parent" | ||
label={ parentPageLabel } | ||
noOptionLabel={ `(${ __( 'no parent' ) })` } | ||
tree={ pagesTree } | ||
selectedId={ parent } | ||
onChange={ onUpdateParent } | ||
/> | ||
); | ||
handleSelection( selection ) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If I enter an empty string in the input, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the feedback @tellthemachines - my intention is definately to combine these PRs by creating our own accessible autocomplete component that wraps/abstracts the underlying external dependency. Part of doing this second PR was finding the common implementation points we need to implement. I welcome any help in that effort! :) It's worth noting for context that the team has already implemented and reviewed accessible autocomplete components from several popular sources, all of which aim for/achieve some degree of accessibility. Of everything we reviewed, this one rose to the top and continues to receive active sustained development. In terms of |
||
const { onUpdateParent } = this.props; | ||
|
||
// Extract the id from the selection. | ||
const matches = selection.match( /.*\(#(\d*)\)$/ ); | ||
if ( matches && matches[ 1 ] ) { | ||
onUpdateParent( matches[ 1 ] ); | ||
} | ||
} | ||
|
||
/** | ||
* Search for pages that match the passed query, passing them to a callback function when resolved. | ||
* | ||
* @param {string} query The search query. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. JSDoc alignment is off. |
||
* @param {Function} populateResults A callback function which receives the results. | ||
*/ | ||
suggestPage( query, populateResults ) { | ||
const { items } = this.props; | ||
|
||
if ( query === items ) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When would the query string ever be equal to the items array? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's also kind of strange how the list displays "no title", but I can't search for that string. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
populateResults( this.resolveResults( items ) ); | ||
return; | ||
} | ||
|
||
if ( query.length < 2 ) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This can be an "or" of the previous conditional. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
populateResults( this.resolveResults( items ) ); | ||
return; | ||
} | ||
|
||
if ( this.searchCache[ query ] ) { | ||
populateResults( this.resolveResults( this.searchCache[ query ] ) ); | ||
return; | ||
} | ||
|
||
this.requestResults( query, populateResults ); | ||
} | ||
|
||
render() { | ||
const { postType, items, onUpdateParent, parent } = this.props; | ||
const { parentPost } = this.state; | ||
let currentParent = false; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's more semantic to use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also keeps the value "typed". |
||
|
||
if ( ! parentPost && false !== parent ) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think What happens when |
||
if ( 0 === parent ) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess this answers my previous question. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. right, for new posts I think its set to 0. |
||
currentParent = ''; | ||
} else { | ||
// We have the parent page id, we need to display its name. | ||
const currentParentFromItems = items && items.find( ( item ) => { | ||
return item.id === parent; | ||
} ); | ||
|
||
// Set or fetch the current author. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks like this comment was copy pasted from somewhere. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. all this code was pulled over from #7385 - will fix |
||
if ( currentParentFromItems ) { | ||
this.setState( { | ||
parentPost: parent, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This sets There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hmmm, let me walk thru the logic again, apologies for the mixup. The postParent should be the full parent post object. |
||
} ); | ||
} else { | ||
this.getCurrentParentFromAPI( parent ); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can't have side effects in functions that should be idempotent like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. good point, this probably needs to happen at mount. |
||
} | ||
} | ||
} | ||
|
||
const isHierarchical = get( postType, [ 'hierarchical' ], false ); | ||
const parentPageLabel = get( postType, [ 'labels', 'parent_item_colon' ] ); | ||
const pageItems = items || []; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. good point |
||
if ( ! isHierarchical || ! parentPageLabel || ! pageItems.length ) { | ||
return null; | ||
} | ||
|
||
if ( false === currentParent ) { | ||
currentParent = parentPost && parentPost.title.rendered ? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This breaks the editor when Try setting the parent to a post in the first 100 and switch to the block panel and back so that it rerenders after There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hmmm, i'll test this. ideally it should re-fetch the correct parent object (on mount) |
||
`${ parentPost.title.rendered } (#${ parentPost.id })` : | ||
`${ __( 'no title' ) } (#${ parentPost.id })`; | ||
} | ||
|
||
if ( items.length > 99 ) { | ||
return ( | ||
<> | ||
<label htmlFor={ parent }>{ __( 'Parent Page' ) }</label> | ||
<Autocomplete | ||
id={ parent } | ||
minLength={ 2 } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we have this here, we don't need the 2nd condition in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. right, i think this one didn't work as expected. |
||
showAllValues={ true } | ||
defaultValue={ currentParent ? currentParent : '' } | ||
displayMenu="overlay" | ||
onConfirm={ this.handleSelection } | ||
source={ this.suggestPage } | ||
showNoOptionsFound={ false } | ||
preserveNullOptions={ true } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this needed? |
||
tStatusQueryTooShort={ ( minQueryLength ) => | ||
// translators: %d: the number characters required to initiate a page search. | ||
sprintf( __( 'Type in %d or more characters for results' ), minQueryLength ) | ||
} | ||
tStatusNoResults={ () => __( 'No search results' ) } | ||
// translators: 1: the index of thre selected result. 2: The total number of results. | ||
tStatusSelectedOption={ ( selectedOption, length ) => sprintf( __( '%1$s (1 of %2$s) is selected' ), selectedOption, length ) } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is there an "s" in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. :) from PHP? I'll remove |
||
tStatusResults={ ( length, contentSelectedOption ) => { | ||
return ( | ||
_n( '%d result is available.', '%d results are available.', length ) + | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess there should be a I think this is also causing an error in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ah - thanks for catching that. |
||
' ' + contentSelectedOption | ||
); | ||
} } | ||
cssNamespace="components-parent-page__autocomplete" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It should be
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
/> | ||
</> | ||
); | ||
} | ||
|
||
const pagesTree = buildTermsTree( pageItems.map( ( item ) => ( { | ||
id: item.id, | ||
parent: item.parent, | ||
name: item.title.rendered ? `${ item.title.rendered } (#${ item.id })` : `${ __( 'no title' ) } (#${ item.id })`, | ||
} ) ) ); | ||
|
||
return ( | ||
<TreeSelect | ||
className="editor-page-attributes__parent" | ||
label={ parentPageLabel } | ||
noOptionLabel={ `(${ __( 'no parent' ) })` } | ||
tree={ pagesTree } | ||
selectedId={ parent } | ||
onChange={ onUpdateParent } | ||
/> | ||
); | ||
} | ||
} | ||
|
||
const applyWithSelect = withSelect( ( select ) => { | ||
|
@@ -49,7 +200,7 @@ const applyWithSelect = withSelect( ( select ) => { | |
const postId = getCurrentPostId(); | ||
const isHierarchical = get( postType, [ 'hierarchical' ], false ); | ||
const query = { | ||
per_page: -1, | ||
per_page: 100, | ||
exclude: postId, | ||
parent_exclude: postId, | ||
orderby: 'menu_order', | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,3 +18,99 @@ | |
width: 66px; | ||
} | ||
} | ||
|
||
.components-parent-page__autocomplete__wrapper { | ||
box-sizing: border-box; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is overwritten by the editor. |
||
position: relative; | ||
} | ||
|
||
.components-parent-page__autocomplete__hint, | ||
.components-parent-page__autocomplete__input { | ||
-webkit-appearance: none; | ||
border: 2px solid; | ||
border-radius: 0; /* Safari 10 on iOS adds implicit border rounding. */ | ||
box-sizing: border-box; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Most of these styles were copy pasted from the library right? Most of the attributes are being overwritten by the editor. This could be slimmed down a lot. I would add them one by one as needed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, the CSS needs work and generalizing especially when we move to use the component in various places. |
||
-moz-box-sizing: border-box; | ||
-webkit-box-sizing: border-box; | ||
margin-bottom: 0; /* BUG: Safari 10 on macOS seems to add an implicit margin. */ | ||
width: 100%; | ||
} | ||
|
||
.components-parent-page__autocomplete__input { | ||
background-color: transparent; | ||
position: relative; | ||
} | ||
|
||
.components-parent-page__autocomplete__hint { | ||
position: absolute; | ||
} | ||
|
||
.components-parent-page__autocomplete__dropdown-arrow-down { | ||
display: none; | ||
} | ||
|
||
.components-parent-page__autocomplete__menu { | ||
background: $white; | ||
border-top: 0; | ||
color: $dark-gray-300; | ||
margin: 0; | ||
padding: #{$grid-size-small / 2} 0; | ||
overflow-x: overlay; | ||
overflow-y: auto; | ||
max-height: 96px; | ||
transition: all 0.15s ease-in-out; | ||
width: calc(100% + 2px); | ||
|
||
&.components-parent-page__autocomplete__menu--visible { | ||
display: block; | ||
} | ||
|
||
&.components-parent-page__autocomplete__menu--hidden { | ||
display: none; | ||
} | ||
} | ||
|
||
.components-parent-page__autocomplete__menu--overlay { | ||
box-shadow: $shadow-popover; | ||
border: $border-width solid $light-gray-500; | ||
left: 0; | ||
position: absolute; | ||
top: 100%; | ||
z-index: 100; | ||
} | ||
|
||
.components-parent-page__autocomplete__menu--inline { | ||
position: relative; | ||
} | ||
|
||
.components-parent-page__autocomplete__option { | ||
font-size: $default-font-size; | ||
padding: #{$grid-size-small / 2} $grid-size; | ||
margin-bottom: 0; | ||
cursor: pointer; | ||
display: block; | ||
position: relative; | ||
|
||
> * { | ||
pointer-events: none; | ||
} | ||
|
||
&:hover { | ||
background: $light-gray-500; | ||
} | ||
|
||
&:focus, | ||
&.components-parent-page__autocomplete__option--focused | ||
&.components-parent-page__autocomplete__option--focused:hover { | ||
background: color(theme(primary) shade(15%)); | ||
color: $white; | ||
outline: none; | ||
} | ||
|
||
&.components-parent-page__autocomplete__option--no-results, | ||
&.components-parent-page__autocomplete__option--no-results:hover { | ||
background: $white; | ||
font-style: italic; | ||
cursor: not-allowed; | ||
} | ||
} |
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 am guessing this is here to make space?
We should remove this and see if we can get away with adding
overflow: visible;
to.components-panel
.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.
yea, this is a temporary kludge. my CSS skills failed here, mentioned in the description - the dropdown needs to expand beyond the bounds of the sidebar here.