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

Jump to bottom

fix: Add defaultValue prop to Search component #2207

Closed
wants to merge 1 commit into from
Closed

fix: Add defaultValue prop to Search component #2207

wants to merge 1 commit into from

Conversation

1Maria
Copy link
Contributor

@1Maria 1Maria commented Oct 20, 2022 •
edited
Loading

Summary

Related Issues or PRs

closes #2206

How To Test

Screenshots (optional)

@1Maria 1Maria requested a review from a user October 20, 2022 02:09
@1Maria 1Maria requested a review from rogeruiz as a code owner October 20, 2022 02:09
@1Maria 1Maria changed the title Add defaultValue prop to Search component fix: Add defaultValue prop to Search component Oct 21, 2022
@haworku
Copy link
Contributor

haworku commented Oct 25, 2022

@1Maria Can you share more about the use case you envision for the "default value" on the search component? Right reading the test it looks like what you would want to use is the placeholder prop to say something like "Search here" in the input before the user types something. Is that what you are looking for?

@haworku haworku mentioned this pull request Oct 25, 2022
Closed
@1Maria
Copy link
Contributor Author

1Maria commented Oct 25, 2022

@haworku the use case I had in mind was for rehydrating state - for example, after a page reload. Is there a different way to accomplish that?

@haworku
Copy link
Contributor

haworku commented Oct 26, 2022

@1Maria Ah, gotcha. Thanks for clarifying, I agree, we need a way to latch into the input for that. I can definitely see the use case where you would want to load a search component with a value already present (like loading a search page based on url with query params or something).

rogeruiz
rogeruiz approved these changes Oct 26, 2022
View reviewed changes
Copy link
Contributor

@rogeruiz rogeruiz left a comment

Choose a reason for hiding this comment

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

lgtm 🌈

Great discussion on this @hannahkanetruss and @1Maria. Also great work on this @1Maria on getting issue #2206 resolved.

@gidjin gidjin mentioned this pull request Oct 28, 2022
Merged
@gidjin
Copy link
Contributor

gidjin commented Oct 28, 2022

In accordance with our notes on contributions I opened a new PR #2211 that will allow the full build suite to run.

@gidjin gidjin closed this Oct 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rogeruiz rogeruiz rogeruiz approved these changes

@gidjin gidjin Awaiting requested review from gidjin

@haworku haworku Awaiting requested review from haworku

@ahobson ahobson Awaiting requested review from ahobson

@brandonlenz brandonlenz Awaiting requested review from brandonlenz

@SirenaBorracha SirenaBorracha Awaiting requested review from SirenaBorracha

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[fix] Search component should support default value
5 participants
@1Maria @haworku @gidjin @rogeruiz @trussworks-infra-zz