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

Add loading indicator to combo box control author selector #68927

Closed
wants to merge 10 commits into from
Closed

Add loading indicator to combo box control author selector #68927

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
f73720d
Add loading indicator to author selector
adamsilverstein Jan 28, 2025
76488c1
Only show "No items found" when isLoading is false
adamsilverstein Jan 29, 2025
3c7baa6
Merge branch 'trunk' into add/combobox-loading-spinner-refresh
adamsilverstein Jan 31, 2025
07690d6
Remove unitended change
adamsilverstein Jan 31, 2025
736c5ff
Update packages/components/src/combobox-control/types.ts
adamsilverstein Jan 31, 2025
fd97afc
Revert "Only show "No items found" when isLoading is false"
adamsilverstein Jan 31, 2025
3a357c4
Only show the suggestion list when the component isn’t loading data
adamsilverstein Jan 31, 2025
374b2b8
Document `isLoading` prop in readme
adamsilverstein Jan 31, 2025
45bbc6b
Add is loading to Combobox storybook
adamsilverstein Jan 31, 2025
cc0e64a
Match readme description to type.js description
adamsilverstein Jan 31, 2025
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
4 changes: 4 additions & 0 deletions packages/components/src/combobox-control/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ import type { TokenInputProps } from '../form-token-field/types';
import { useDeprecated36pxDefaultSizeProp } from '../utils/use-deprecated-props';
import { withIgnoreIMEEvents } from '../utils/with-ignore-ime-events';
import { maybeWarnDeprecated36pxSize } from '../utils/deprecated-36px-size';
import Spinner from '../spinner';

const noop = () => {};

Expand Down Expand Up @@ -126,6 +127,7 @@ function ComboboxControl( props: ComboboxControlProps ) {
help,
allowReset = true,
className,
isLoading = false,
Copy link
Contributor

Choose a reason for hiding this comment

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

Adding this prop won't affect existing usages, although I wonder if it's a good idea, API-wise.

The component doesn't have a canonical example of how to use it with asynchronously loaded options, and I wonder if adding this prop will improve the DevX (related to #55574) — @WordPress/gutenberg-components what do you think?

If we want to encourage this pattern, we should at least add a Storybook example about it?

Copy link
Member Author

@adamsilverstein adamsilverstein Jan 31, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I wonder if adding this prop will improve the DevX

Not sure about DevX, this change is about improving User Experience. As is, the combobox selector provides a poor UX when the internet connection is slow or unreliable. The user has no way of knowing that a search is underway during the async search. This shortcoming was overlooked when we originally built the component and I feel this more of a bug than an enhancement.

messages = {
selected: __( 'Item selected.' ),
},
Expand Down Expand Up @@ -362,6 +364,7 @@ function ComboboxControl( props: ComboboxControlProps ) {
onChange={ onInputChange }
/>
</FlexBlock>
{ isLoading && <Spinner /> }
{ allowReset && (
<Button
size="small"
Expand Down Expand Up @@ -396,6 +399,7 @@ function ComboboxControl( props: ComboboxControlProps ) {
__experimentalRenderItem={
__experimentalRenderItem
}
isLoading={ isLoading }
adamsilverstein marked this conversation as resolved.
Show resolved Hide resolved
/>
) }
</div>
Expand Down
5 changes: 5 additions & 0 deletions packages/components/src/combobox-control/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,4 +86,9 @@ export type ComboboxControlProps = Pick<
* If passed, the combobox input will show a placeholder string if no values are present.
*/
placeholder?: string;

/**
* When loading, combobox will show a spinner
adamsilverstein marked this conversation as resolved.
Show resolved Hide resolved
*/
isLoading?: boolean;
};
2 changes: 2 additions & 0 deletions packages/components/src/form-token-field/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ export function FormTokenField( props: FormTokenFieldProps ) {
__experimentalAutoSelectFirstMatch = false,
__nextHasNoMarginBottom = false,
tokenizeOnBlur = false,
isLoading = false,
} = useDeprecated36pxDefaultSizeProp< FormTokenFieldProps >( props );

if ( ! __nextHasNoMarginBottom ) {
Expand Down Expand Up @@ -743,6 +744,7 @@ export function FormTokenField( props: FormTokenFieldProps ) {
onHover={ onSuggestionHovered }
onSelect={ onSuggestionSelected }
__experimentalRenderItem={ __experimentalRenderItem }
isLoading={ isLoading }
/>
) }
</div>
Expand Down
3 changes: 2 additions & 1 deletion packages/components/src/form-token-field/suggestions-list.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ export function SuggestionsList<
suggestions = [],
displayTransform,
instanceId,
isLoading,
__experimentalRenderItem,
}: SuggestionsListProps< T > ) {
const listRef = useRefEffect< HTMLUListElement >(
Expand Down Expand Up @@ -157,7 +158,7 @@ export function SuggestionsList<
);
/* eslint-enable jsx-a11y/click-events-have-key-events */
} ) }
{ suggestions.length === 0 && (
{ suggestions.length === 0 && ! isLoading && (
Copy link
Member

Choose a reason for hiding this comment

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

The SuggestionsList component is shared, which is why we're passing the isLoading prop, correct?

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

<li className="components-form-token-field__suggestion is-empty">
{ __( 'No items found' ) }
</li>
Expand Down
6 changes: 6 additions & 0 deletions packages/components/src/form-token-field/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,11 @@ export interface FormTokenFieldProps
* @default false
*/
tokenizeOnBlur?: boolean;

/**
* Is the component loading data?
*/
isLoading?: boolean;
}

/**
Expand All @@ -207,6 +212,7 @@ export interface SuggestionsListProps<
displayTransform: ( value: T ) => string;
instanceId: string | number;
__experimentalRenderItem?: ( args: { item: T } ) => ReactNode;
isLoading?: boolean;
}

export interface TokenProps extends TokenItem {
Expand Down
6 changes: 4 additions & 2 deletions packages/editor/src/components/post-author/combobox.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,8 @@ export default function PostAuthorCombobox() {
const [ fieldValue, setFieldValue ] = useState();

const { editPost } = useDispatch( editorStore );
const { authorId, authorOptions } = useAuthorsQuery( fieldValue );
const { authorId, authorOptions, isLoading } =
useAuthorsQuery( fieldValue );

/**
* Handle author selection.
Expand All @@ -44,13 +45,14 @@ export default function PostAuthorCombobox() {
<ComboboxControl
__nextHasNoMarginBottom
__next40pxDefaultSize
label={ __( 'Author' ) }
label={ __( 'Authorz' ) }
Copy link
Member

Choose a reason for hiding this comment

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

Typo?

Copy link
Member Author

Choose a reason for hiding this comment

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

ha, this was me trying to get my build working and not seeing changes. will revert!

options={ authorOptions }
value={ authorId }
onFilterValueChange={ debounce( handleKeydown, 300 ) }
onChange={ handleSelect }
allowReset={ false }
hideLabelFromVision
isLoading={ isLoading }
Copy link
Contributor

Choose a reason for hiding this comment

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

My preference would we to focus this PR solely on the ComboboxControl component.

We should add a Storybook example here to allow for easy testing of the component in isolation.

In a separate, follow-up PR, we could then tweak the post author combobox to use the new prop

Copy link
Member Author

Choose a reason for hiding this comment

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

The entire point of this PR is to improve the author selector - changing only the combobox won't actually change anything, shouldn't we aim for PRs that actually improve things?

I can look into adding to the storybook, would this just be a variation with the isLoading prop?

Copy link
Contributor

Choose a reason for hiding this comment

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

The @wordpress/component package is not only used in the Gutenberg repository, but it's also used by third-party developers. Therefore, we usually try to keep changes to the "library" separate from changes to the Gutenberg app (hence why I suggest testing the changes in isolation in Storybook).

Having smaller and more focused PRs is also easier to review, and understand for a future developer stumbling upon it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Having smaller and more focused PRs is also easier to review, and understand for a future developer stumbling upon it.

I understand the principal, however this entire PR is currently less than 35 lines modified so I think is already easy to review and understand! Given the testing instructions I provided, I feel having the PR do something makes it easier to test and understand.

That said, I sense that you feel strongly about this and I will defer to your more recent experience building here. I will split the combobox and author changes into separate PRs.

Copy link
Member Author

Choose a reason for hiding this comment

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

I created a follow up PR with just the Comobox changes here:

#68990

This is a follow up PR to introduce the post author changes:

#68991

Let's continue the conversation on those PRs.

/>
);
}
7 changes: 4 additions & 3 deletions packages/editor/src/components/post-author/hook.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,9 @@ import { store as editorStore } from '../../store';
import { AUTHORS_QUERY, BASE_QUERY } from './constants';

export function useAuthorsQuery( search ) {
const { authorId, authors, postAuthor } = useSelect(
const { authorId, authors, postAuthor, isLoading } = useSelect(
( select ) => {
const { getUser, getUsers } = select( coreStore );
const { getUser, getUsers, isResolving } = select( coreStore );
const { getEditedPostAttribute } = select( editorStore );
const _authorId = getEditedPostAttribute( 'author' );
const query = { ...AUTHORS_QUERY };
Expand All @@ -30,6 +30,7 @@ export function useAuthorsQuery( search ) {
authorId: _authorId,
authors: getUsers( query ),
postAuthor: getUser( _authorId, BASE_QUERY ),
isLoading: isResolving( 'getUsers', [ query ] ),
};
},
[ search ]
Expand Down Expand Up @@ -68,5 +69,5 @@ export function useAuthorsQuery( search ) {
return [ ...currentAuthor, ...fetchedAuthors ];
}, [ authors, postAuthor ] );

return { authorId, authorOptions, postAuthor };
return { authorId, authorOptions, postAuthor, isLoading };
}
Loading