-
Notifications
You must be signed in to change notification settings - Fork 36
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
Refactor Searchbar into Smaller Components #237
Refactor Searchbar into Smaller Components #237
Conversation
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 noticed that closing the search bar on mobile and reopening it preserves the query that was entered previously. This felt a little unexpected, so just wanted to point it out—perhaps Allison has an opinion on it!
`; | ||
|
||
const CondensedSearchbar = ({ onExpand }) => ( | ||
<ExpandButton aria-label="Open MongoDB Docs Search" onClick={onExpand}> |
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.
Sorry for missing this the first time around, but why is the Oops, was a bit scrambled. What I meant is: What is the purpose of including the onClick
handler attached here instead of to the ExpandMagnifyingGlass
IconButton
?ExpandMagnifyingGlass
icon when we are using an IconButton
?
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.
Ah the button wraps around the icon and this is where the click handler is meant to be bound. This actually also increases the radius where a user can click and something can happen as well.
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.
Ah! So with the IconButton we pass the Icon as a child. We use styled
here because I use it as a selector when styling the ExpandButton
. It is really just an Icon
but it has to be a styled(Icon) to use the selector as such.
const SEARCHBAR_DESKTOP_WIDTH = '372px'; | ||
const SEARCHBAR_HEIGHT = '36px'; |
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.
Good call standardizing these 💯
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.
Yeah it was bothering me haha
oh interesting! This is the intended behavior but we could clear out when closed. It might be nice if they close by accident or something to keep the last query |
Thanks! |
Ecosystem Staging
Prior Branch Ecosystem Staging
Guides Staging
This PR eliminates some building technical debt by splitting the
Searchbar
component into four components:Searchbar
parent which acts as an overall controller of the experienceCondensedSearchbar
which is the magnifying glass to pop-open the searchbarExpandedSearchbar
which is the searchbar openSearchTextInput
which is the custom LeafyGreen text inputThese were all in one component before and it was getting tricky to manage state and control changes. This approach is much more maintainable and clean! Passing state now also feels more natural as well.
I'll bump the snapshots separately since this is large enough. I looked side by side on this branch versus the old one (and on mobile). The only change is on mobile the magnifying glass while searching is always filled in. This is actually what the design calls for anyway. No other looks or functionality should change.