-
Notifications
You must be signed in to change notification settings - Fork 47
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
pkp/pkp-lib#6675 Added extra features to the Autosuggest field (showP… #171
base: main
Are you sure you want to change the base?
pkp/pkp-lib#6675 Added extra features to the Autosuggest field (showP… #171
Conversation
…agination, minInputLength, maxSelectedItems, replaceWhenFull) and a new component (FieldPagedAutosuggest) to enter a custom data mapper to feed the suggestions model.
I'm really sorry, but I think I'm going to say no to pagination in the autosuggest field. I've had a look at widely used autosuggest patterns and I haven't seen pagination implemented much. I did find a couple of libraries that supported pagination through an infinite scroll technique (ie - load more as they scroll), but this has its own usability problems. Because autosuggest is already a tricky component for accessibility, I'm not willing to commit to more UI complexity in it. And I especially don't want to adopt an unusual pattern that hasn't really been proven in the wild. The autosuggest patten has a few common conventions and one of them is that you continue to type to refine your search. If the list is so large that this doesn't provide a good UX, then we need to redesign the UI to not use an autosuggest. But I would only want to do that in response to UX research that showed this was a problem. I can suggest an alternate way forward here. First, we should cap the height of the suggestions container and let it scroll, so that a large number of items in the list doesn't require scrolling away from the input field. See this example (search for numbers). Then, when more than one page of suggestions exists, the component can continue to make requests for other pages in the background and add them to the list as they come in. So the list of suggestions would grow as subsequent pages were loaded in. Your addition of the loading spinner is great. And that will help here too. The spinner should remain visible as subsequent pages are loaded in. I suspect we'll have some accessibility issues around this, but it will likely be the same with or without the spinner. The one trick here is that you'll want to make sure that you bail when the user's search changes. You don't want to be gathering pages and pages of results for an obsolete search phrase. |
Another approach which I've considered, but haven't yet researched, is to provide a "Load More" button at the end of the results. The autosuggest component has a footer slot that might be used for this. The trick is that we'll need to be careful about how we use the aria attributes to link the button to the input field. My first thought is that we'll need an |
No worries, I just wanted to close the issue fast with a generic solution 😁 I'm not able to tell which option is more accessible, I'd personally opt-in for the infinite scroll (the less I click, the better), as I'll be able to keep typing and scroll at the same time. Then about loading, I'd load 2-3 pages in advance (until it generates enough content to cover |
I'd like to avoid automatic infinite scroll. Some of the usability problems are described in this article from nngroup. In this case, I'm concerned about two issues they describe: frustration from changing scroll lengths and the sense of completion people get from finishing a "page". In general, I almost always prefer the information on the page to change as a result of an intentional action. That's why the mockup above has users click on a button to view more. If you're worried about needing to click again and again to browse large lists, you can increase the number of results that are returned in subsequent requests. In this case, the initial list would be short and return quickly, but the subsequent list could be long (100 items) and slow to load. Users would then learn they have two strategies: keep typing for quick results, or load a bunch at once and scroll. |
This site is an old pearl! About the frustration I think people must be already used to it, given the amount of doom scrollers out there 😂 Well, as long as we're not loading the whole result set, I'm ok with any UI... So let's go with the "Load more". |
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.
This looks really good, @jonasraoni. I've left some comments throughout, and I'm really happy with how this has come along. However, I've found a real blocker in terms of a11y and I'm not sure how to work around it. Maybe you'll have a better idea.
A key part of the accessibility handling is the markup that relates the <input>
field to the role="listbox"
, and implements required keyboard controls (up/down/enter) to keep the interaction accessible. However, the Load More button can't be accessed through these controls. The user must use tab to go to the next thing to reach the button (or use the mouse).
autosuggest-click-to-load-more-fixed.mp4
Tab seems easy enough. But as far as I can tell, there's no common, accepted way to indicate to a screen reader that more controls for this list can be accessed with a tab. I've looked at some examples (gov.uk, WAI combobox), but I don't see any examples of an accessible approach here.
I know you've put a lot of work into this. But I don't want to add a feature to the autosuggest that I know will not be accessible.
This may be a case where it's better to step back and think about an alternate approach to the UI problem. We may need something more like a ListPanel for enrolling a user as a reviewer. That said, I'm open to exploring this further. I just did some quick research. But if you can find a trustworthy example of implementing a load more button like this in an accessible way, we can pursue it further. I think, aside from this, the implementation was looking good.
.sort( | ||
new Intl.Collator( | ||
(this.localeKey || $.pkp.app.currentLocale).split('_')[0] | ||
).compare |
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'd rather do this sorting on the server side.
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 agree! I just saw the list wasn't sorted and wanted to address it temporarily, until the pkp/pkp-lib#7529 is addressed :)
@@ -26,13 +31,14 @@ import FieldBaseAutosuggest from './FieldBaseAutosuggest.vue'; | |||
export default { | |||
extends: FieldBaseAutosuggest, | |||
methods: { | |||
setSuggestions(newItems) { | |||
this.suggestions = newItems.map(item => { | |||
setSuggestions(items, itemsMax) { |
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.
The use of newVal
and new***
is a Vue.js convention that I'd like to preserve. If you wanted you could call them newSuggestions
and newItemsMax
, but either way keep the new
in there.
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.
No problem =]
@@ -104,7 +104,7 @@ export default { | |||
...fieldBaseAutosuggest, | |||
apiUrl: '/usernames.json', | |||
name: 'assignedTo', | |||
label: 'Assigned To Editors', | |||
label: 'Assigned To Editors (limited to 3 items)', |
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.
This limit shouldn't apply in the submissions list panel, either in the preview or in the actual implementation.
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.
Yeah, I just wanted to leave an example using the property somewhere.
@@ -90,7 +92,11 @@ export default { | |||
// discard responses that are outdated. | |||
this.latestGetRequest = $.pkp.classes.Helper.uuid(); | |||
|
|||
$.ajax({ | |||
if (this.lastRequest) { | |||
this.lastRequest.abort('Aborted due to new user request'); |
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.
No message is required here. We can abort silently.
import FieldSelectUsers from '@/components/Form/fields/FieldSelectUsers.vue'; | ||
import FieldSelectIssues from '@/components/Form/fields/FieldSelectIssues.vue'; | ||
export default { | ||
extends: Filter, | ||
components: { | ||
FieldAutosuggestPreset, | ||
FieldMappedAutosuggest, |
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.
Is this used anywhere in a FilterAutosuggest
?
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 used in the enroll reviewer interface, but as you asked to create a custom component, this will be removed.
import FieldBaseAutosuggest from './FieldBaseAutosuggest.vue'; | ||
|
||
export default { | ||
name: 'FieldMappedAutosuggest', |
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.
Instead of creating a generic component that takes a mapping function as a prop, I'd prefer to use a specific component built for this use case. All of the other autosuggest fields are set up to use setSuggestions
as the mapping function, and by the time it is compiled it won't add much to the build size to have a separate component for each.
So let's call this EnrollUserAsReviewerAutosuggest
and move the mapping function into the setSuggestions
method.
@@ -404,7 +457,9 @@ export default { | |||
if (newVal === oldVal) { | |||
return; | |||
} | |||
this.getSuggestions(); | |||
if (newVal.length >= this.minInputLength) { |
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.
This prevents suggestions from updating when I delete characters. When the input value drops below minInputLength
, go ahead and empty the suggestions array.
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 saw this problem, but I forgot to fix 😁
} else { | ||
this.suggestions = suggestions; | ||
} | ||
this.itemsMax = itemsMax; |
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.
Rather than extend every autosuggest field with pagination, let's only support it where we need it. We shouldn't need to manage offset or pagination in any other autosuggest field.
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.
Agreed! I had to cancel the pagination in the controlled vocabs component, but the other way would feel more natural.
selectedLabel: 'Assigned', | ||
apiUrl: '/usernames.json' | ||
apiUrl: '/usernames.json', | ||
maxSelectedItems: 3 |
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.
Set up a new preview component if you want to demonstrate the maxSelectedItems
prop. You don't need to for merge, though.
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.
Ok!
pkp/pkp-lib#6675 Minor adjustments to autosuggest styles when loading…
…e keyboard navigation and fixed the auto-scrolling.
Hi @NateWr, I worked a bit on this yesterday and I had success to make the load more accessible through the keyboard. |
Note: the "load more" will be at the end of the list, but it's doable to implement your design (load more fixed on the bottom). |
I like it, @jonasraoni! I'm not 100% confident about the a11y but I think it's got a good chance of working out alright. Here are a couple of things that still need working out based on a quick test in the UI Library:
In an ideal world, I'd like to see the whole load more functionality moved out of the base autosuggest component into its own component. That will make it easier to clean up and manage if we run into a11y problems, and limit the potential for it to be used where it's not needed. It may be easier said than done, though. Extending component templates is a pain. However, if you may be able to get this to work by using a slot inside the
Give that a shot and see if it works in the |
…agination, minInputLength, maxSelectedItems, replaceWhenFull) and a new component (FieldPagedAutosuggest) to enter a custom data mapper to feed the suggestions model.