ComboboxControl: add a spinner when isLoading. #27865
Conversation
adamsilverstein
requested review from
ajitbohra and
chrisvanpatten
as code owners
|
@youknowriad This adds a loading spinner while the dropdown is searching; any ideas why the |
|
Size Change: +130 B (0%) Total Size: 1.07 MB
ℹ️ View Unchanged
|
|
@youknowriad when you have a moment, I would love your help figuring out why the |
|
@youknowriad going to take one more look to see why this doesn't work as expected. |
|
@youknowriad Got it, changing the selector in This is ready for review/testing. |
|
Awesome, design wise it doesn't seem great though. I wonder if we could have some thoughts from designers cc @jasmussen @kellychoffman |
|
What's an easy way to reproduce this? The combobox doesn't appear when I only have two users, it looks like. Is the combobox in use in other places? I just need to be able to invoke the spinner so I can position it with a little CSS. |
|
I think you need at least 10 users, alternatively, just force the isLoading to true (change it in code) |
|
There's some CSS-in-JS styles going on for this spinner:
CC: @ItsJonQ Because of that, I'm not sure what the best way is to style them. In the mean time, here's some CSS that changes this:
to this:
Change this: to this: |
@jasmussen Thanks for this, will add for now. |
|
Added suggested CSS, looks much better now! Screen.Recording.2021-01-22.at.12.09.33.PM.mp4 |
|
Looks good. Just one issue with the width of the input+dropdown jumping around. On focus, the input is full width. The width then jumps around when you start typing and again when the spinner appears. Can we keep it full width on focus? |
I agree, it would probably look best if this was always full width. I'm not exactly sure how to do this. I notice the element has an extra layer of wrapping in the dom: seems to be caused by the If I remove this div or set its width to 100% it looks like this: Screen.Recording.2021-01-22.at.03.45.38.PM.mp4@youknowriad do you have an idea where that |
|
Abouti the full-width control, I commented on this ticket as well. I think it's probably a good idea, but in the resting state it gives a bit of extra prominence that isn't warranted. Can we try the suggestion I posted and see if it works? That is: keep the initial width when resting, then animate to full-width when focused? |
|
I just realized I restated Kelly's comment verbatim:
Apologies, for some reason I read it as "full width always". I blame that it's Monday. Consider this an emphatic vote of confidence, if this is possible! |
@jasmussen thanks for clarifying! I will work on this. Do we have any other elements that have a similar "animate width on activating" behavior? |
I don't know that we have, but I could imagine it being a part of #27331, for example for padding controls. I've seen it quite often for search input fields, such as https://www.w3schools.com/howto/howto_css_animated_search.asp. If you run into trouble ping me and I'll see if I can help! |
|
@jasmussen - I tried adding the transition style to the input field without much luck. Can you help me out with the exact CSS change that is needed here when you have a chance? Please feel free to commit directly to the branch :) |
|
Absolutely. I'm out for the weekend but will take a look Monday. |
|
I took a look at this, and I can tell why you had such a headache getting this to work. As it turns out, That's fine, but flex items behave quite differently. In this case, the default flex direction is "row", which means any divs inside get flipped on their side, i.e. they grow in width rather than height. It's a bit mind-bendy. But it also means that the fact that the box is full width in this picture, is a coincidence, and not one we can replicate:
If I change those rules to be
The bigger problem is that in order to handle the flex behavior, we need to be able to target the right elements. We can't do that right now, because the markup is generic: 👆 that empty div up there is wreaking havoc. It needs to be either
So, question: can that fragment (I'm assuming the empty div is a fragment) be easily removed? (or replaced with If yes, then I can fix it. If no, we might want to ship this one as is and ticket a followup, or watch the refactor in #27331. |
@jasmussen I think I looked at trying to remove this |
| @@ -35,7 +35,7 @@ function PostAuthorCombobox() { | |||
| authorId: getEditedPostAttribute( 'author' ), | |||
| postAuthor: author, | |||
| authors: getAuthors( query ), | |||
| isLoading: isResolving( 'core', 'getAuthors', [ query ] ), | |||
| isLoading: isResolving( 'getAuthors', [ query ] ), | |||
There was a problem hiding this comment.
this no longer works as is, investigating further
|
Closing in favor of #68927 which is now working. I still some issues I'd like to resolve before review (namely the drop-down shows "No items found." during the search, a bug clearly visible currently when throttling the search callback). |
Description
Add a spinner loading indicator to the
ComboboxControlwhen theisLoadingprop is true.This can be used to inform the user that a search is taking place, which is not obvious when you are typing currently (and may be even more confusing over slower networks).
Fixes #25798
Helpful wp-cli commands for testing:
wp user generate --role=editor --count=250wp user delete $(wp user list --role=editor --field=ID) --reassign=2How has this been tested?
Screenshots
Types of changes
Note, when I was testing this, I discovered that the
isLoadingattribute set by thePostAuthorcomponent is never true even during searches. Something isn't working with this call itisResolving:gutenberg/packages/editor/src/components/post-author/index.js
Line 37 in f2f89ed
Checklist: