-
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
refactor(catalog): introduce usePathname hook, remove React Router dependency from the Catalog #1276
Conversation
…pendency from the Catalog
✅ Deploy Preview for veda-ui ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
The |
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 tested the changes in both the Veda UI and the Next.js repository as well as on the GHG Center side. Everything works as expected with no regressions.
* @returns {string} The current `pathname`. Returns an empty string during SSR | ||
* or if the `window` object is unavailable. | ||
*/ | ||
export const usePathname = () => { |
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.
👍
Yeah, that seems likely. For example the GHG Center instance is running under |
@dzole0311 thanks for the review! Let's keep an eye on that instance once we roll out the changes. |
Related Ticket: Contributes to #1108
Description of Changes
react-router
dependency from the Catalog componentsusePathname
hookNotes & Questions About Changes
The
pathname
prop is passed down for two things:veda-ui/app/scripts/components/common/catalog/catalog-card.tsx
Line 126 in 820730f
veda-ui/app/scripts/components/common/catalog/filters-control.tsx
Line 57 in 820730f
I suspect that the usage on item 1 was intented to cover the use case of an instance running on a subpath (e.g.
https://myinstance.org/veda
), why is hard to replicate locally but this change shouldn't affect that.Validation / Testing
Using
veda-config
:Using
next-veda-ui
instance:This is ready for review.