-
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
Combine hooks to hopefully deprecate useBrowserControls #1022
Conversation
✅ Deploy Preview for veda-ui ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
}; | ||
} | ||
|
||
export function useCatalogViewQS() { |
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 pushed a change that separates out QS-related states and actions into a separate hook. How do you think? @sandrahoang686 ? I think in this way, we don't have to trigger the initialization of atoms, and make it clearer what is controlling the query parameter.
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.
yep, I mentioned this in the description, it does look cleaner too, good with me! I opened this PR because I didn't want to merge to your branch directly in case you were cleaning so feel free to merge whenever comfortable
I noticed that if you go to the data stories, open a data story, and click on the topic. at the top like you would in a dataset, that you are returned to the data story list with the correct URL parameters but without the filtered view. It seems the data story component doesn't handle the URL parameter like the data catalog component does. I see this same behavior in production, so it might not be in scope for this PR, but we should be aware of it if it's not a quick fix. |
@aboydnw this is a pr off of the Hanbyuls main pr to help consolidate code but you are right, stories and catalog use different pieces of logic, we should consolidate that at some point as well and go towards hopefully decprecating that old logic that stories is using |
Thanks for the great find @aboydnw ! As @sandrahoang686 said, this PR doesn't introduce anything new. But it looks like that might be a quick fix (and will help code consolidation). I will handle that problem in my branch. @sandrahoang686 I am pretty sure the ts-check failure is related to my original PR. I will handle that in my branch. Thanks for your contribution! |
@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 logiconCatalogAction
.We can actually start deprecating and removing all
useBrowserControl
logic, right nowstories/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 logiconCatalogAction
.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