Port the Catalog View in the E&A dataset selector modal #989
Conversation
✅ Deploy Preview for veda-ui ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
dzole0311
requested review from
hanbyul-here,
sandrahoang686 and
faustoperez
and removed request for
hanbyul-here and
sandrahoang686
dzole0311
changed the title
|
🤔 I am not sure I am complicating the matter or not - but I am unsure if we need URLs to be manipulated when users interact through modal. Especially dinvr the query parameters are not getting reset when the datasets are seleted. In a scenario like below
In Step 3, that user will see the taxonomies that were not chosen by the user. The current behavior makes sense in a way that the same component does the same URL manipulation. On the other hand, I think mixing up URL parameters from two components (data catalog modal and e&a components) for one page (new E&A) creates an edge case like the above. I also see much of the need for URL sharing while selecting the datasets for the E&A page. What do you think? Am I missing anything? It is awesome that the URL manipulation logic is captured in the hook: https://github.com/NASA-IMPACT/veda-ui/pull/989/files#diff-469d42d5232f537ff54ffb1ea3ee80fe574196d55379cf93c482bb6171cee567R52 Maybe we can use the hook only for Data Catalog page? |
app/scripts/components/exploration/components/dataset-selector-modal/index.tsx
Show resolved
Hide resolved
| action: CatalogActions, | ||
| value: any, | ||
| taxonomies: any, | ||
| setSearch: (value: string) => void, |
There was a problem hiding this comment.
I realize that we would need more of this type of utility function - which takes either setState or setAtomas a setter function - like the one that we use for reconciling STAC metadata here: https://github.com/NASA-IMPACT/veda-ui/pull/988/files#diff-b0d6bd8ff29016fd81054f35bfcc97dd5b50077310a81f6043a99240cb43a4eaR167
I'd like to ask your thoughts how we should type these util functions moving forward. Should we explicitly type the setter function as either setAtom or setState? ex. I tried this method in https://github.com/NASA-IMPACT/veda-ui/pull/988/files#diff-b0d6bd8ff29016fd81054f35bfcc97dd5b50077310a81f6043a99240cb43a4eaR169 - and I am still unsure if this just added complicacy or not. But I also think any is a bit too broad. Any thoughts?
There was a problem hiding this comment.
I think both can be verbose, even for simpler types like a setter for a string (ie. React.Dispatch<React.SetStateAction<string>>).
How about we create aliases for the setters and use that? For example: type SetState<T> = Dispatch<SetStateAction<T>> and then we can use it like this:
const [count, setCount]: [number, SetState<number>] = useState(0);
There was a problem hiding this comment.
Yeah, aliases sound great. I will leave it up to you if you want to apply this change to this pr or not.
There was a problem hiding this comment.
Wooh, yes, all the problems I mentioned are gone! (And I was actually not aware that the modal was manipulating the urls 😓 My apologies! - but I do think the current behavior makes more sense url-wise. It is great that you kept the functionality with local state!) I left some comments, but most of them are not blockers!
- comment for the future: this filter function is actually a great feature to have a unit test 😬
Good point, added :) |
| action: CatalogActions, | ||
| value: any, | ||
| taxonomies: any, | ||
| setSearch: (value: string) => void, |
There was a problem hiding this comment.
Yeah, aliases sound great. I will leave it up to you if you want to apply this change to this pr or not.
|
Flagging that I missed the |
…nd new E&A page (#1021) **Related Ticket:** #1016 ### Description of Changes This PR makes Catalog View on the data-catalog page use QS-state for query parameters, while Catalog View on the new E&A page uses Jotai. The current Atoms related to URL management is not designed for cross-page navigation. I drew a diagram to describe the navigation problem between catalog page and overview page.  More context: We moved our query parameter management to use Jotai because of the concerns about the mix-use of QS-State and Jotai for query parameters on the same page. (Specifically - for the new E&A page, other parameters are managed by Jotai). More details on this PR that initially introduced data-catalog view to new E&A page: #989 (comment) In the end, we ditched the Query parameter manipulation thinking that we don't need. (Details are in this comment: #989 (comment)) But the bug reported #1016 made me realize that was a wrong call. ### Notes & Questions About Changes - We need a better solution for long-term url management. ### Validation / Testing Navigate back and forth from data catalog to overview, from overview to data catalog (especially through taxonomy pills on the overview page) Navigate to the new E&A page through the 'Explore Data' button on the overview page. I opened a pr on ghg for testing: US-GHG-Center/veda-config-ghg#421
Related Ticket: 973
Description of Changes
Ported the CatalogView to the E&A modal.
Notes & Questions About Changes
{Add additonal notes and outstanding questions here related to changes in this pull request}
Validation / Testing