Use different query parameter managements between data-catalog page and new E&A page #1021
Conversation
✅ Deploy Preview for veda-ui ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
@hanbyul-here it might be cleaner to just still use the `useCatalogView` hook. Just moved the qs-state logic over here instead and exposed it from this hook since Actions are the exact same, it can reference the same action logic `onCatalogAction`. We can actually start deprecating and removing all `useBrowserControl` logic, right now `stories/hub` is still using it but we can probably port it over. This way we have one hook managing separate states and exposing them which I think is better than having duplicated logic. We could create a separate hook though to make it more explicit but both can use the same actions logic `onCatalogAction`. I know we probably dont want to use qs-state-hook. We can probably work on updating that logic later or in this PR whichever you see fit. Feel free to fix naming.. naming isn't great here, was just trying to combine and see if this is better
hanbyul-here
force-pushed
the
1016-search-qp
branch
from
e38dfa1 to
3627d39
Compare
hanbyul-here
force-pushed
the
1016-search-qp
branch
from
3627d39 to
ba5b9db
Compare
hanbyul-here
force-pushed
the
1016-search-qp
branch
from
4680166 to
b518fb9
Compare
|
This PR is ready for the review @sandrahoang686 I ended up with a few more changes to fix the bug that @aboydnw reported in #1022 I deprecated
|
|
I tried to break the Dataset overview -> exploration link, adding/editing layers, and the stories catalog. The only thing that I noticed is the following:
This is different from how it behaves in prod. I could see arguments for both experiences, but I wanted to call it out in case we intentionally wiped filters in the old layer selection modal. cc @faustoperez |
hanbyul-here
force-pushed
the
1016-search-qp
branch
from
d177321 to
37a0eb0
Compare
|
Thank you @aboydnw , that is a sharp catch! That behavior you described, is precisely why we ditched 'keeping the current state of filtering as URL parameters' at first (Details are: #989 (comment)). However, we learned we needed it to manipulate the current filter state through query parameters. (So the modal can show the filtered layers on landing when a user comes from the Data Overview page.) But I agree that it is a weird behavior that the search state persists after the purpose of filtering is fulfilled - so I added 'reset' of search terms and taxonomies whenever the layer selections are confirmed, or the layer selection modal is closed (hidden). |
app/scripts/components/common/browse-controls/use-browse-controls.ts
Outdated
Show resolved
Hide resolved
| commit: navigate | ||
| }); | ||
|
|
||
| const [search, setSearch] = useQsState.memo( |
There was a problem hiding this comment.
💭 : Can we make a tech-debt note or todo where we comment on how we hope to consolidate how we manage taxonomies states and url. But here's a thought, I think it actually makes sense to keep this separate? right now the atomWithUrlStability manages taxonomies state but url routing which does it make sense to break up? So it manages just that state but does nothing with the url, we can create a hook or have a useEffect wherever applicate to worry about the naviagation based on whatever state change? Does that make sense?
There was a problem hiding this comment.
Oh, That sounds like how the current main branch is using the filters with the states in the data selection modal, but in a separate hook. Am I getting it right?
Aside from how we will handle the query parameter in the future... I wonder if there is a better place than the code base where this kind of discussion should go so it can get deserving attention. I changed @NOTE prefix to @TECH-DEBTto signal that actions need to be taken.
There was a problem hiding this comment.
Not sure how its managed in the modal but yep talking about not combining taxonomies state and url changes likes whats happening in the atomWithUrlStability and to have them separate
| } | ||
| // @NOTE: A hook using qs-state for query parameter management for cross-page navigation | ||
| // Details: https://github.com/NASA-IMPACT/veda-ui/pull/1021 | ||
| export function useCatalogViewQS(): UseCatalogViewResult { |
There was a problem hiding this comment.
⛏️ : What do you think about changing the name to useFiltersUrlManagement because this is mostly tied to the filters instead of the greater catalog view. We can probably change the interface to be UseFiltersUrlManagementResults... its long though..
There was a problem hiding this comment.
I wanted to get around without renaming the hook, but you are right that this hook is not capturing what it is doing now 💦 I renamed it as useFilterswithURLAtom and useFiltersWithQS, hoping that the name of the hook signals enough what it does!
There was a problem hiding this comment.
(This also required renaming of related variables to prefix by Filter, instead of Catalog !)
|
QA Findings
|
hanbyul-here
force-pushed
the
1016-search-qp
branch
from
dd66ca7 to
2785f5b
Compare
## What's Changed Note: This could be a patch version number. But the fix for the query parameter on the data catalog page, new E&A page, and stories hub brought a substantial change and improvement, in my opinion. And there are some other improvements we made. So I am cutting a minor version release. ## 🎉 Features 🦗 ## 🚀 Improvements * Create bug.md in #1015 * Use VizDataset data type for block map, set a default value for interactive in #1006 ## 🐛 Fixes * Fix scrollytelling projection problem in #1011 * Use different query parameter managements between data-catalog page and new E&A page in #1021 **Full Changelog**: v5.1.2...v5.2.0
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
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