(feat)O3-5068: darken placeholder and focus input on empty search #2005
Conversation
|
@ibacher Could you please review this PR? If you’re happy with the changes, I’d appreciate it if you could add the hacktoberfest-accepted label to this PR, as I’m participating in Hacktoberfest. Thanks a lot for your support! I’m also open to making any further refinements you suggest. |
|
@denniskigen Why does it feel like only my PRs are getting hit by these flaky tests? 😭 |
|
Awesome! 🎉 the E2E tests are passing now. |
| <Search | ||
| autoFocus | ||
| className={styles.patientSearchInput} | ||
| className={`${styles.patientSearchInput} ${isInputClicked ? styles.darkPlaceholder : ''}`} |
There was a problem hiding this comment.
We generally use the classnames library for stuff like this:
| className={`${styles.patientSearchInput} ${isInputClicked ? styles.darkPlaceholder : ''}`} | |
| className={classNames('styles.patientSearchInput', { [styles.darkPlaceholder]: isInputClicked })} |
| const [searchTerm, setSearchTerm] = useState(initialSearchTerm); | ||
| const [isInputClicked, setIsInputClicked] = useState(false); | ||
| const responsiveSize = isCompact ? 'sm' : 'lg'; | ||
| const inputRef = useRef(null); |
There was a problem hiding this comment.
So modifying the ref here makes this (a component that exposes a ref a breaking change. This is because the component-user supplied ref will now not point to the DOM element.
To fix this, we need to add the following:
useImperativeHandle(ref, () => inputRef.current!)This allows us to use a component-internal ref and properly expose it to consumers.
| if (isInputClicked) { | ||
| const timeout = setTimeout(() => { | ||
| setIsInputClicked(false); | ||
| }, 5000); |
There was a problem hiding this comment.
5 seconds feels quite long for this interaction. I'd maybe make it 3 seconds tops. The idea is to catch the users eye.
There was a problem hiding this comment.
Thanks will make it 3 sec.
|
|
||
| const handleSubmit = useCallback( | ||
| (event: React.FormEvent<HTMLFormElement>) => { | ||
| (event) => { |
There was a problem hiding this comment.
Not sure why this removes the type?
There was a problem hiding this comment.
Thanks ! will get it back.
| } else { | ||
| setIsInputClicked(true); | ||
| inputRef.current.focus(); |
There was a problem hiding this comment.
Again, from my comments, I suggested only triggering this on a click on the button. Putting this is the submit handler also triggers when the user hits enter and that's exactly the thing we wanted to avoid.
There was a problem hiding this comment.
I understand your point, but I don’t see a strong reason to exclude the Enter key. The user already needs to click the search icon with their mouse to access the search bar. Once it’s active, pressing Enter feels like a natural alternative way to trigger the advanced search results. Moreover, if the user clicks anywhere outside the search bar, it disappears—so the Enter action wouldn’t interfere with other interactions.
There was a problem hiding this comment.
once the user has already clicked the search icon and started typing, it’s more convenient for them to simply press Enter rather than reaching for the mouse again(during that timeframe). It feels like a natural flow from a usability perspective.
There was a problem hiding this comment.
I don't disagree that it's a natural flow. The question is does the user need the box to be highlighted if they press enter without typing anything? That feels like an accidental keypress rather than an operation the user was trying to perform, so it's better if the system does nothing. OTOH, the user clicking the search button seems like a non-accidental action, so it makes more sense to call attention to why nothing happened.
| } | ||
|
|
||
| .darkPlaceholder ::placeholder { | ||
| color: #393939 !important; |
There was a problem hiding this comment.
Not sure where this came from. Use $text-02 or import the Carbon theme and use theme.$text-primary so this aligns with our standard colors. We generally don't invent colors if possible.
There was a problem hiding this comment.
Also, instead of just changing the color here, maybe we use a 1s animation to fade between the current color and the new color...
E.g.,
.darkPlaceholder ::placeholder {
animation: fade-dark 1s;
}
@keyframes fade-dark {
0% {}
100% { color: $text-02 !important; }
}Something along those lines...
|
If removing the Enter key is truly necessary, I’ll be happy to include that change in the next commit. |
|
Thanks for the thoughtful review @ibacher ! If it’s not too much trouble, could you please add the hacktoberfest-accepted label to this PR? I’m participating in Hacktoberfest and would really appreciate it. Thank you so much for your time and support! 🙏 |
3 participants
Requirements
Summary
This PR addresses the issue of no visual feedback when a user clicks the search button with an empty search term. To improve user experience, the following changes have been implemented:
These changes provide a clear visual cue to the user that the search term is missing.
Screenshots
simple.1.mp4
Related Issue
https://openmrs.atlassian.net/browse/O3-5068
Other
None.