-
Notifications
You must be signed in to change notification settings - Fork 6
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
Data Catalog enhancement with floating filter sidebar #918
Conversation
✅ Deploy Preview for veda-ui ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
@faustoperez @j08lue this is still WIP but I could use an early user flow review of the new filters menu (cards are still actively being worked on and there are some quirks I still need to figure with the filters). A few questions and things to note which I wanted to flag right now.
|
Great progress @sandrahoang686 ! 🙌
Not sure I'm following but the expected behavior is that multiple parameters can be selected at the same time.
I think it should be closed by default.
We could put the filter tags at the top of the dataset results column to avoid this. I have other comments about spacing, hover states, paddings, etc., please let me know when the work is finished to review those 👍 . Thanks! |
Checkin 4/18 Update:
|
I would say additive within earth filter, exclusive between them, i.e. |
I know the AND/OR logic really added complexity wrt routing... Hope it will not be too much headache to resolve it. A quick note - an important feature was that the filter sidebar should be fixed, so it is visible as you scroll down longer lists of datasets. Just wanted to be sure that you are aware of this detail, @sandrahoang686, so it does not come up only at the very end. 🙏 |
42c9731
to
44bc781
Compare
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 can do more review but that might not be what this change needs at this point.
Two blockers I see from this PR are 1. https://github.com/NASA-IMPACT/veda-ui/pull/918/files#r1578769216 2. https://github.com/NASA-IMPACT/veda-ui/pull/918/files#diff-d569f507e8f543c166ce6a250b3c64a0b1b997d8d59ee996e0d390d3af4636bdR288 (See the review comment) If you have bandwidth, check this change in GHG context to see if there is anything you didn't intend is happening there.
There are improvements we can bring regarding styles, I left comments about hard-coded values. I can see that the page will benefit from subtle animations since there are many moving pieces on the page. These can be followed up while people are testing the main functionality, so once two blockers are cleared, feel free to merge it.
|
||
const FilterMenu = styled.div` | ||
border: 2px solid ${themeVal('color.base-200')}; | ||
border-radius: 4px; |
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.
Does this need to be this hard-coded number? Other elements of Card is using (themeVal('shape.rounded'), 2)
for its border-radius value and it might increase the consistency of the style if we stick with using themeVal
value.
const FilterMenu = styled.div` | ||
border: 2px solid ${themeVal('color.base-200')}; | ||
border-radius: 4px; | ||
padding: 12px; |
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.
Same with padding and margins. Across the application, glsp
or variableGlsp
are used for spacing.
/> | ||
|
||
<FeaturedDatasets /> | ||
<BrowseSection> |
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.
{topics.map((t) => ( | ||
/> | ||
<VerticalDivider variation='light' /> | ||
{/* TODO: Implement modified date: https://github.com/NASA-IMPACT/veda-ui/issues/514 */} |
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.
(Not introduced by this PR but) might be time to just get rid of this and close this issue
|
||
const ControlsWrapper = styled.div<{ width?: string; }>` | ||
min-width: 20rem; | ||
width: ${props => props.width ?? '3rem'}; |
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.
Am I reading right that width is smaller than min-width? then dos this style do anything?
Good call, @hanbyul-here. I created a GHG Center feature preview here: |
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.
The EMIT dataset is missing in the GHG Center preview of this feature. I have a suspicion that this is related to the disableExplore: true
status of this dataset.
US-GHG-Center/veda-config-ghg#340 (comment)
We need to make sure all datasets are shown in the main catalog.
From my limited understanding, it seems like the catalog uses veda-ui/app/scripts/components/exploration/data-utils.ts Lines 48 to 59 in 4c98710
which is an extended version of veda-ui/app/scripts/components/exploration/data-utils.ts Lines 40 to 42 in 4c98710
For the main catalog, we should not apply that filter. |
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.
All datasets now show up in the main catalog and it looks nice both in our mock and the GHG Center preview. Good to go!
## 🎉 Features - Zoom in AOI, TOI when analysis is run in #906 - Add custom javascript injection #846 - ADR for V2 Refactor: #875 - Open all external links in a new tab in #870 - Include dataset id to filter layers in #910 - Some datasets can only be analyzed with layers from the same source in #913 - Create minimal partial data layer scaffold starting off with Data Catalog for VEDA2 Refactor in #893 - Add analysis preset in #921 ## 🚀 Improvements - Chart style improvement in #903 - Data Catalog enhancement with floating filter sidebar in #918 - Sum as statistics option in #925 - ## 🐛 Fixes - Sort featured stories based on publication date in descending order in #907 - Replace latency with temporalResolution in layer info in #898 - Add a workaround for Safari scroll problem in #909 - Handle empty result in #922
Related Ticket: #915
Description of Changes
This pull request implements the new Data Catalog View with the filters sidebar
Notes & Questions About Changes
Validation / Testing
Shoulds:
Should Nots:
/exploration?datasets=%5B%5D
view