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

feat(DualListSelector, SearchInput): use SearchInput in DLS, add type prop to SearchInput #8040

Merged
merged 5 commits into from
Sep 30, 2022

Conversation

kmcfaul
Copy link
Contributor

@kmcfaul kmcfaul commented Sep 22, 2022

What: Closes #7977

Updates DualListSelector to internally build a SearchInput.
Adds type prop to SearchInput to match DualListSelector's initial input type.

@patternfly-build
Copy link
Contributor

patternfly-build commented Sep 22, 2022

Copy link
Contributor

@thatblindgeye thatblindgeye left a comment

Choose a reason for hiding this comment

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

Looks good! Something that would be worth a followup since it might be a little out of scope here is that the Search Input should be passed onClear callback so that a focusable "clear" button renders (right now an icon is rendered when you type into the DLS search, but you can't focus it)

I can open a followup for that, unless you'd want to try sneaking it into this PR.

@kmcfaul
Copy link
Contributor Author

kmcfaul commented Sep 23, 2022

I can sneak an update for that in. The default behavior will call onChange with an empty string and may be overridden with onSearchInputClear.

Though I'm a little confused about how some of these search callbacks are meant to work, because several are hidden from the props documentation with @hide, and it doesn't look like onSearch (which is visible) is used anywhere. Is the new prop I added onSearchInputClear actually necessary if a user is meant to ignore all these props and use searchInput instead for composable dual list selectors?
@nicolethoen

@mmenestr
Copy link
Collaborator

This looks great! The only thing I'd suggest is adding an empty state if the search doesn't match any of the options? I realize that is an oversight from when I designed this. But maybe just a small or xs empty state using the text and icon here: https://www.patternfly.org/v4/components/empty-state#no-match-found

@nicolethoen
Copy link
Contributor

nicolethoen commented Sep 23, 2022

This looks great! The only thing I'd suggest is adding an empty state if the search doesn't match any of the options? I realize that is an oversight from when I designed this. But maybe just a small or xs empty state using the text and icon here: https://www.patternfly.org/v4/components/empty-state#no-match-found

That may be a pretty significant enhancement to the Dual list selector at this point. I am in the process of a dual list selector re-work and if you open an issue to incorporate an empty state, I can include it with the re-work

@mmenestr
Copy link
Collaborator

@nicolethoen sounds good, just opened this: #8080

Copy link
Contributor

@mcoker mcoker left a comment

Choose a reason for hiding this comment

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

lgtm! 🥳

@kmcfaul
Copy link
Contributor Author

kmcfaul commented Sep 30, 2022

Rebased conflict

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dual list selector: Search should use search input component
7 participants