-
Notifications
You must be signed in to change notification settings - Fork 78
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
LF-3750 Add search to dashboard #2945
Conversation
…archResults for use in dashboard
} | ||
|
||
.dateRangeSelector { | ||
max-width: 360px; |
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.
This is actually larger than what is shown in Figma, but I was worried about the text not fitting.
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.
Working great, it's beautiful! ❤️ I left a couple of small comments
packages/webapp/src/components/PopupFilter/PureCollapsibleSearch/index.jsx
Show resolved
Hide resolved
<div className={styles.circleContainer}> | ||
<div className={styles.circle} /> | ||
</div> | ||
<SearchIcon className={styles.searchIcon} onClick={onSearchOpen} /> |
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.
Here the onClick
should actually be on the button instead of the icon -- currently if you try to click on the little edges of the button right outside of the icon, even though it seems clickable it won't open the search.
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.
THANK YOU!!! This is why the onClick was mysteriously failing at times 😁
setModalStyle({ | ||
position: 'absolute', | ||
top: `${rect.top}px`, | ||
left: ref === searchRef.current ? undefined : `${rect.left}px`, |
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 don't understand the bit where it checks if the ref is actually the searchRef and sets different values in that case, can you explain that case a bit more? 🙏
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.
If a containerRef
isn't provided from the parent component, this sets the position of the modal searchbar directly on top of the search button -- so it grabs a vertical position, but does not grab a horizontal position or width (as the horizontal position and width of the button itself is irrelevant). Not supplying a left
keeps the modal default of centering horizontally, and the searchbar width will always be 95vw
.
I think overlaying the button is a pretty good default (so that a container ref doesn't always have to be created in the parent), but if it makes the code too unreadable, I can remove it. Alternatively... if you have a suggestion for writing the same in a more readable style, that would be great!
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.
That makes sense to me! I'd keep it and if possible just add a small comment indicating if a container ref isn't passed in the position will be calculated to overlay the button 🙂
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.
Looks great! I left one small comment!
@@ -78,4 +80,8 @@ const FinanceDateRangeSelector = () => { | |||
); | |||
}; | |||
|
|||
FinanceDateRangeSelector.PropTypes = { |
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.
There is a warning about the "P" in "PropTypes" being capitalized!
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.
Thank you!
Description
This PR creates the new
<PureCollapsibleSearchbar />
component (Storybook) and adds it to the Finances dashboard.Note:
<TextButton />
native transparent takes precedent). If you know why that is, could you let me know 🙏Jira link: https://lite-farm.atlassian.net/browse/LF-3750
Type of change
How Has This Been Tested?
Checklist: