Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Changes from 2 commits
ea1913b
7930922
a7c2e48
b4ddc11
2527793
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
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.
Add a spinner when loading 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.
Call this
moreSuggestionsCount
and update the docblock to read: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 like to get rid of the
canReplace
functionality. I think it will be confusing for users, who may not notice that an existing selection has been replaced.Instead, when the max count has been reached, set
inputProps.readonly
to true and make sure the suggestions are empty. Then the user will have to delete their existing selection to search for a new one. I know it's a little bit more clicking, but less likely to cause user error this way.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 thought about renaming it to
singleSelection: Boolean
, then it would address my use case.Another option would be to show a warning message "Only N items can be selected", but I saw the errors are injected from outside and didn't want to mess with adding extra messages.
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 😁
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 :)
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 thesetSuggestions
method.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.