empty search bar, implementation #42
Conversation
Display message to the user if an empty search is performed #37: *added react-bootstrap dependecy *added modal logic to detect empty search bar *added modal view to display the modal iteself *handled logic to search when search bar is not empty *added onClick method to current search button
| if (search) { | ||
| this.props.handleSearch(search) | ||
|
|
||
| if(search.length === 0){ |
There was a problem hiding this comment.
First review. I don't know if you prefer to be guided with just a code review or some workable code as well. Either way I'll provide both and you can choose.
Issue in onSearchClick conditional :
search values like
' 'bypass your modal because their length are > 0 you don't have any guards for it, thus falling through to you else branch.
Possible Fix
onSearchClick = e => {
const search = this.state.search.trim()
if (search) {
this.props.handleSearch(search)
}
e.preventDefault()
}There was a problem hiding this comment.
thank you for the review @cdrani. i certainly do appreciate the guidance and the workable code as well, i'm able to implement these changes immediately, but will hold off a day to see if any of the team members input anything more on the conversation (towards the bottom, after this).
| if (search) { | ||
| this.props.handleSearch(search) | ||
|
|
||
| if(search.length === 0){ |
There was a problem hiding this comment.
Hmmm... the modal isn't a bad idea, but it is kinda obtrusive as you can't just click or tab out of it to close it. I'm sure react-bootstrap Modals already have this accessibility feature? It also kinds of blends into the dark background of the cover image.
A suggestion I want to make to replace the Modal is with an Alert component. Alert already has warning, danger, etc color using its variant prop. Additionally we can have it fade out on its own after a few second.
Here's a sandbox of what I mean. I think this route would be easier to interact with and visually appear better. Also now we can start using React's hooks.
What do you guys think?
// @JasonFritsche @PabloRosas17
There was a problem hiding this comment.
@cdrani I agree, this implementation could be made better by using an Alert. Also, how do you feel about a message that is displayed for a set amount of seconds only?
Thanks @cdrani && @PabloRosas17
There was a problem hiding this comment.
Check the sandbox. Press enter with empty search and the alert appears and then disappears after two seconds.
There was a problem hiding this comment.
i've ran the sandbox, i think it's a fantastic idea. although i did find the pop up modal a bit attractive and can see some room for growth on it. i was thinking, do the sandbox first as it is the cleanest and instant solution. however, for the future i'd like to reference this issue by going the modal route. i'm thinking if we use a popup modal due to an empty search, perhaps we can create a custom suggestions search.
something like this:
Oops! No empty searches allowed, but check out some of our recommended (pubs/bars/wineries) in your area.
Thank you @cdrani @JasonFritsche
There was a problem hiding this comment.
for now i will rework this issue using an alert, does that work with the team?
please also let's create an issue for my suggestion above if you like what i'm saying,
should i include a diagram to illustrate my suggestion in an abstract form? will it help?
There was a problem hiding this comment.
-
Yes, full agreement on Alert all around. Please proceed.
-
I'm not sold on the Modal. I still think it's unneeded - especially anywhere near a search form.
- What if someone just forgets to enter input? Is the modal supposed to show up every time? That would be annoying and distracting having to interact with it each time. Recommendations should be passive not aggressive.
- We don't need a modal to showcase recommended places. This can just be added anywhere in below the search form.
- The api is America centralized, therefore the location based recommendations would only benefit people already in the area.
- However, I can see adding recommendations in a Create Component for Brewery detail/map #9 (details page) that would be based on what the user has just searched.
-
I'm not the one to be convinced anyways, these are just my opinions. I would still love to see the diagram.
There was a problem hiding this comment.
@cdrani I agree with your arguments here. @PabloRosas17 Let's go forward without the modal. Thanks for your efforts. I look forward to seeing your work.
There was a problem hiding this comment.
your explanation is clarifying @cdrani , let's toss the modal aside.
sounds good @JasonFritsche , i'll spectate for now.
There was a problem hiding this comment.
@PabloRosas17: Are you working on this currently? We could free this up to someone else or I could complete it if you don't have time. Just let us know. Thanks.
|
This branch is far behind and it doesn't seem any further work will be added to it. PR #57 was created to address this issue. Closing now. |
Display message to the user if an empty search is performed #37:
*added react-bootstrap dependecy
*added modal logic to detect empty search bar
*added modal view to display the modal iteself
*handled logic to search when search bar is not empty
*added onClick method to current search button