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

Parent Page Selector: use an accessible autocomplete/search component #16666

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions package-lock.json

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
Expand Up @@ -29,10 +29,15 @@ export function PageAttributes( { isEnabled, isOpened, onTogglePanel, postType }
onToggle={ onTogglePanel }
>
<PageTemplate />
<PageAttributesParent />
<PanelRow>
<PageAttributesParent />
</PanelRow>
<PanelRow>
<PageAttributesOrder />
</PanelRow>
<PanelRow>
Copy link
Contributor

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.

Copy link
Member Author

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.

<p />
</PanelRow>
</PanelBody>
</PageAttributesCheck>
);
Expand Down
1 change: 1 addition & 0 deletions packages/editor/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
"@wordpress/url": "file:../url",
"@wordpress/viewport": "file:../viewport",
"@wordpress/wordcount": "file:../wordcount",
"accessible-autocomplete": "^1.6.2",
"classnames": "^2.2.5",
"inherits": "^2.0.3",
"lodash": "^4.17.14",
Expand Down
199 changes: 175 additions & 24 deletions packages/editor/src/components/page-attributes/parent.js
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 {
Copy link
Contributor

Choose a reason for hiding this comment

The 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?

Copy link
Member Author

Choose a reason for hiding this comment

The 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 ) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

This should use core-data's getEntityRecords.

Copy link
Contributor

Choose a reason for hiding this comment

The 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?

Copy link
Member Author

Choose a reason for hiding this comment

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

probably for ease of adding the debounce?

Copy link
Contributor

Choose a reason for hiding this comment

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

this.requestResults = debounce( this.requestResults.bind( this ) ); follows the pattern better.

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,
Copy link
Contributor

Choose a reason for hiding this comment

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

It's more semantic to use undefined for uninitialized values and null for intentional absence of values.

Copy link
Member Author

Choose a reason for hiding this comment

The 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 ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This should use core-data's getEntityRecord.

Copy link
Member Author

Choose a reason for hiding this comment

The 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 ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

If I enter an empty string in the input, selection here is undefined, so it's effectively impossible to unset a parent page once it's been set. We should be able to handle an empty string as a no parent option.

Copy link
Member Author

Choose a reason for hiding this comment

The 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 Preact as a requirement - that sounds like a bug or build issue - we will be sure to remove that as a dependency, the project does support Preact but does not require it.

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.
Copy link
Contributor

Choose a reason for hiding this comment

The 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 ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

When would the query string ever be equal to the items array?

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

Choose a reason for hiding this comment

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

When would the query string ever be equal to the items array?
That looks incorrect, it is supposed to be detecting when the search term hasn't changed. I can fix that.

populateResults( this.resolveResults( items ) );
return;
}

if ( query.length < 2 ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be an "or" of the previous conditional.

Copy link
Member Author

Choose a reason for hiding this comment

The 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;
Copy link
Contributor

Choose a reason for hiding this comment

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

It's more semantic to use undefined for uninitialized values and null for intentional absence of values.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also keeps the value "typed".


if ( ! parentPost && false !== parent ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think parent is ever the Boolean false. It might be falsy when not set, but not false.

What happens when parent changes and parentPost is already set?

if ( 0 === parent ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this answers my previous question. parent is 0 when not set?

Copy link
Member Author

Choose a reason for hiding this comment

The 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.
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this comment was copy pasted from somewhere.

Copy link
Member Author

Choose a reason for hiding this comment

The 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,
Copy link
Contributor

Choose a reason for hiding this comment

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

This sets parentPost to an ID, but this.getCurrentParentFromAPI sets it to an object?

Copy link
Member Author

Choose a reason for hiding this comment

The 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 );
Copy link
Contributor

Choose a reason for hiding this comment

The 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 render.

Copy link
Member Author

Choose a reason for hiding this comment

The 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 || [];
Copy link
Contributor

Choose a reason for hiding this comment

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

pageItems is not really needed here. ! items would have the same effect.

Copy link
Member Author

Choose a reason for hiding this comment

The 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 ?
Copy link
Contributor

Choose a reason for hiding this comment

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

This breaks the editor when parentPost is set to an ID because currentParentFromItems was found.

Try setting the parent to a post in the first 100 and switch to the block panel and back so that it rerenders after items are loaded.

Copy link
Member Author

Choose a reason for hiding this comment

The 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 }
Copy link
Contributor

Choose a reason for hiding this comment

The 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 handleSelection.

Copy link
Member Author

Choose a reason for hiding this comment

The 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 }
Copy link
Contributor

Choose a reason for hiding this comment

The 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 ) }
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is there an "s" in $s?

Copy link
Member Author

Choose a reason for hiding this comment

The 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 ) +
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess there should be a sprintf() here to wrap the _n() and actually pass the length value for replacement within the strings.

I think this is also causing an error in handleSelection(), as the passed selection is undefined. The error goes away when adding the sprintf() above.

Copy link
Member Author

Choose a reason for hiding this comment

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

ah - thanks for catching that.

' ' + contentSelectedOption
);
} }
cssNamespace="components-parent-page__autocomplete"
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

The 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 ) => {
Expand All @@ -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',
Expand Down
96 changes: 96 additions & 0 deletions packages/editor/src/components/page-attributes/style.scss
Original file line number Diff line number Diff line change
Expand Up @@ -18,3 +18,99 @@
width: 66px;
}
}

.components-parent-page__autocomplete__wrapper {
box-sizing: border-box;
Copy link
Contributor

Choose a reason for hiding this comment

The 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;
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

Choose a reason for hiding this comment

The 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;
}
}