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

Include search params in toolpad navigation #4537

Open
ckopsa opened this issue Dec 12, 2024 · 3 comments
Open

Include search params in toolpad navigation #4537

ckopsa opened this issue Dec 12, 2024 · 3 comments
Labels
bug 🐛 Something doesn't work

Comments

@ckopsa
Copy link

ckopsa commented Dec 12, 2024

I attempted to use a link that included search params in the global navigation for toolpad, but it was slicing of the search params.

/jobs?page=2 --> /jobs

Here is the diff that solved my problem:

diff --git a/node_modules/@toolpad/core/shared/Link.js b/node_modules/@toolpad/core/shared/Link.js
index cfe8842..a50dba5 100644
--- a/node_modules/@toolpad/core/shared/Link.js
+++ b/node_modules/@toolpad/core/shared/Link.js
@@ -21,7 +21,7 @@ export const Link = /*#__PURE__*/React.forwardRef(function Link(props, ref) {
     return event => {
       event.preventDefault();
       const url = new URL(event.currentTarget.href);
-      routerContext.navigate(url.pathname, {
+      routerContext.navigate(url.pathname + url.search, {
         history
       });
       onClick?.(event);

Search keywords:

@github-actions github-actions bot added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Dec 12, 2024
@bharatkashyap
Copy link
Member

Makes sense to me! Would you be interested in contributing a PR?

@apedroferreira Wdyt?

@bharatkashyap bharatkashyap added bug 🐛 Something doesn't work good first issue Great for first contributions. Enable to learn the contribution process. and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Dec 13, 2024
@Janpot
Copy link
Member

Janpot commented Dec 18, 2024

I don't think we should assume all query parameters are valid for each path and should always transfer. IMO, clearing them as we do now should be the default behavior.

We can think about designing an API that allows whitelisting certain parameters. I'm not sure yet how such API should look like. I can definitely imagine there are other scenarios where we want to let users intercept the navigation logic and have it alter behavior based on where they're navigating from/to or other conditions. Maybe we need more use cases?

@apedroferreira
Copy link
Member

apedroferreira commented Dec 18, 2024

I don't think we should assume all query parameters are valid for each path and should always transfer. IMO, clearing them as we do now should be the default behavior.

We can think about designing an API that allows whitelisting certain parameters. I'm not sure yet how such API should look like. I can definitely imagine there are other scenarios where we want to let users intercept the navigation logic and have it alter behavior based on where they're navigating from/to or other conditions. Maybe we need more use cases?

True, by default it would be wrong to always transfer URL params, but it is something we're even doing in Toolpad Studio for some specific parameters, so we could add a feature for it. Probably with a property in certain navigation items...

By the way, the way we're doing this in Toolpad Studio is:

const [searchParams] = useSearchParams();

const retainedSearch = React.useMemo(() => {
  for (const name of searchParams.keys()) {
    if (!RETAINED_URL_PARAMS.has(name)) {
      searchParams.delete(name);
    }
  }

  return searchParams.size > 0 ? `?${searchParams.toString()}` : '';
}, [searchParams]);
  
const navigation = React.useMemo(
    () =>
      pages.map(({ slug, displayName }) => ({
        segment: `pages/${slug}${retainedSearch}`,
        title: displayName,
      })),
    [pages, retainedSearch],
);

@apedroferreira apedroferreira removed the good first issue Great for first contributions. Enable to learn the contribution process. label Dec 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work
Projects
None yet
Development

No branches or pull requests

4 participants