Skip to content
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

Remove React Router's useNavigate dependency #1270

Merged
merged 4 commits into from
Nov 25, 2024

Conversation

vgeorge
Copy link
Contributor

@vgeorge vgeorge commented Nov 21, 2024

Related Ticket: Contributes to #1108

This aims to ensure catalog filters work correctly in Next.js.

Description of Changes

Removes the usage of React Router's useNavigate hook in the library to make it more agnostic of routing frameworks, allowing it to work better with Next.js instances.

Changes included:

  • Added a custom commit function to useQsStateCreator. It appears this was the only place where the useNavigate function was effectively in use. I tried using the default commit function from the qs-state-hook library, but it doesn't work with Next.js
  • Formatted the touched files

Validation / Testing

Using veda-config:

  1. Visit the data catalog page
  2. Click on filters, and URL params are updated as usual
  3. Reload the page with filters selected; the filter panel is populated as expected
  4. Repeat the steps on the Exploration and Stories pages

Using next-veda-ui instance:

  1. Link the source of this PR to the Next.js instance using the instructions in veda-ui/docs/development/REGISTRY.md
  2. Replace the file app/(datasets)/data-catalog/catalog.tsx (this avoids build errors caused by type issues)
'use client';
import React from 'react';
import { CatalogView, useFiltersWithQS } from '@lib';
import { usePathname } from 'next/navigation';
import Link from 'next/link';

export default function Catalog({ datasets }: { datasets: any }) {
  const pathname = usePathname();
  const controlVars = useFiltersWithQS();

  return (
    <CatalogView
      datasets={datasets}
      onFilterChanges={() => controlVars}
      pathname={pathname}
      linkProperties={{
        LinkElement: Link,
        pathAttributeKeyName: 'href',
      }}
    />
  );
}
  1. Access http://localhost:3000/data-catalog. Filter changes are reflected in the URL, and reloading populates the filter panel as expected

This is ready for review.

Copy link

netlify bot commented Nov 21, 2024

Deploy Preview for veda-ui ready!

Name Link
🔨 Latest commit 34d878f
🔍 Latest deploy log https://app.netlify.com/sites/veda-ui/deploys/673f3e8e1186fd0008b7241a
😎 Deploy Preview https://deploy-preview-1270--veda-ui.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Collaborator

@dzole0311 dzole0311 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me @vgeorge

Did some (smoke) tests using the standalone veda ui:

  1. Filters + search query work and are persisted as before in the Data Catalog
  2. Clearing them all works
  3. Smoke tested the routing via clicking on the "Explore Data" works as before (opens the E&A with the applied filter)
  4. Smoke tested the catalog in the E&A
  5. Hard reloading works as before

Same for the next.js instance (except the step 3 above, we don't have the Explore buttons yet)

@vgeorge vgeorge merged commit edc5d9e into main Nov 25, 2024
9 checks passed
@vgeorge vgeorge deleted the 1108-remove-use-navigate-dependency branch November 25, 2024 10:21
dzole0311 added a commit that referenced this pull request Nov 26, 2024
### 🚀 Improvements
* Exploration & Analysis core VEDA feature breakout
#1251
#1154
* Introduce `EnvConfigProvider` to simplify the injection of environment
variables from host applications like Next.js
#1253
* Remove React Router's `useNavigate` dependency for simplifying the
routing #1270

### 🐛 Fixes
* Fix an issue where card images fail to maintain the correct aspect
ratio #1265
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants