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

Menu component with routes rerenders all the components on every route #5941

Closed
gayathrirajendran opened this issue Feb 12, 2024 · 13 comments · Fixed by #5949
Closed

Menu component with routes rerenders all the components on every route #5941

gayathrirajendran opened this issue Feb 12, 2024 · 13 comments · Fixed by #5949
Assignees
Labels
Resolution: Workaround Issue or pull request contains a workaround. It needs to be reviewed further by Core Team Type: Bug Issue contains a defect related to a specific component.
Milestone

Comments

@gayathrirajendran
Copy link

Describe the bug

On click of the routes in primereact Menu component, the entire component set refreshes/rerenders, also noticeable is a flicker on the screen. The same does not happen with the routes in unordered list. If this is intended behavior, can you guide to the behavior where a menu with internal route link is possible i.e not have the entire page refreshed

Reproducer

https://codesandbox.io/p/github/gayathrirajendran/test-menu-route/master?layout=%257B%2522sidebarPanel%2522%253A%2522EXPLORER%2522%252C%2522rootPanelGroup%2522%253A%257B%2522direction%2522%253A%2522horizontal%2522%252C%2522contentType%2522%253A%2522UNKNOWN%2522%252C%2522type%2522%253A%2522PANEL_GROUP%2522%252C%2522id%2522%253A%2522ROOT_LAYOUT%2522%252C%2522panels%2522%253A%255B%257B%2522type%2522%253A%2522PANEL_GROUP%2522%252C%2522contentType%2522%253A%2522UNKNOWN%2522%252C%2522direction%2522%253A%2522vertical%2522%252C%2522id%2522%253A%2522clsi6joce0006356i6szovvcj%2522%252C%2522sizes%2522%253A%255B70%252C30%255D%252C%2522panels%2522%253A%255B%257B%2522type%2522%253A%2522PANEL_GROUP%2522%252C%2522contentType%2522%253A%2522EDITOR%2522%252C%2522direction%2522%253A%2522horizontal%2522%252C%2522id%2522%253A%2522EDITOR%2522%252C%2522panels%2522%253A%255B%257B%2522type%2522%253A%2522PANEL%2522%252C%2522contentType%2522%253A%2522EDITOR%2522%252C%2522id%2522%253A%2522clsi6jocd0002356imcfehp90%2522%257D%255D%257D%252C%257B%2522type%2522%253A%2522PANEL_GROUP%2522%252C%2522contentType%2522%253A%2522SHELLS%2522%252C%2522direction%2522%253A%2522horizontal%2522%252C%2522id%2522%253A%2522SHELLS%2522%252C%2522panels%2522%253A%255B%257B%2522type%2522%253A%2522PANEL%2522%252C%2522contentType%2522%253A%2522SHELLS%2522%252C%2522id%2522%253A%2522clsi6jocd0004356ix2upa9k4%2522%257D%255D%252C%2522sizes%2522%253A%255B100%255D%257D%255D%257D%252C%257B%2522type%2522%253A%2522PANEL_GROUP%2522%252C%2522contentType%2522%253A%2522DEVTOOLS%2522%252C%2522direction%2522%253A%2522vertical%2522%252C%2522id%2522%253A%2522DEVTOOLS%2522%252C%2522panels%2522%253A%255B%257B%2522type%2522%253A%2522PANEL%2522%252C%2522contentType%2522%253A%2522DEVTOOLS%2522%252C%2522id%2522%253A%2522clsi6jocd0005356i0uxjal7c%2522%257D%255D%252C%2522sizes%2522%253A%255B100%255D%257D%255D%252C%2522sizes%2522%253A%255B50%252C50%255D%257D%252C%2522tabbedPanels%2522%253A%257B%2522clsi6jocd0002356imcfehp90%2522%253A%257B%2522id%2522%253A%2522clsi6jocd0002356imcfehp90%2522%252C%2522tabs%2522%253A%255B%255D%257D%252C%2522clsi6jocd0005356i0uxjal7c%2522%253A%257B%2522id%2522%253A%2522clsi6jocd0005356i0uxjal7c%2522%252C%2522activeTabId%2522%253A%2522clsi6qhhq00s4356izwelmw90%2522%252C%2522tabs%2522%253A%255B%257B%2522type%2522%253A%2522TASK_PORT%2522%252C%2522taskId%2522%253A%2522dev%2522%252C%2522port%2522%253A5173%252C%2522id%2522%253A%2522clsi6qhhq00s4356izwelmw90%2522%252C%2522mode%2522%253A%2522permanent%2522%252C%2522path%2522%253A%2522%252Fsample2%2522%257D%255D%257D%252C%2522clsi6jocd0004356ix2upa9k4%2522%253A%257B%2522id%2522%253A%2522clsi6jocd0004356ix2upa9k4%2522%252C%2522activeTabId%2522%253A%2522clsi6nlyq006p356ih8o4psld%2522%252C%2522tabs%2522%253A%255B%257B%2522id%2522%253A%2522clsi6jocd0003356i9wz6fq8v%2522%252C%2522mode%2522%253A%2522permanent%2522%252C%2522type%2522%253A%2522TERMINAL%2522%252C%2522shellId%2522%253A%2522clsi6jo8f000vdki468zvglar%2522%257D%252C%257B%2522type%2522%253A%2522TASK_LOG%2522%252C%2522taskId%2522%253A%2522CSB_RUN_OUTSIDE_CONTAINER%253D1%2520devcontainer%2520templates%2520apply%2520--template-id%2520%255C%2522ghcr.io%252Fdevcontainers%252Ftemplates%252Ftypescript-node%255C%2522%2520--template-args%2520%27%257B%257D%27%2520--features%2520%27%255B%255D%27%2522%252C%2522id%2522%253A%2522clsi6nkjl004n356ix0i0czq1%2522%252C%2522mode%2522%253A%2522permanent%2522%257D%252C%257B%2522type%2522%253A%2522TASK_LOG%2522%252C%2522taskId%2522%253A%2522dev%2522%252C%2522id%2522%253A%2522clsi6nlyq006p356ih8o4psld%2522%252C%2522mode%2522%253A%2522permanent%2522%257D%255D%257D%257D%252C%2522showDevtools%2522%253Atrue%252C%2522showShells%2522%253Atrue%252C%2522showSidebar%2522%253Atrue%252C%2522sidebarPanelSize%2522%253A15%257D

PrimeReact version

14.x.x

React version

18.x

Language

TypeScript

Build / Runtime

Vite

Browser(s)

No response

Steps to reproduce the behavior

  1. Navigate on menu, notice flicker
  2. Navigate on list, notice no flicker

Expected behavior

  1. Navigate on menu and get no flicker/no complete rerender
@gayathrirajendran gayathrirajendran added the Status: Needs Triage Issue will be reviewed by Core Team and a relevant label will be added as soon as possible label Feb 12, 2024
@melloware
Copy link
Member

Not related to PrimeReact. Your issue is how React Router Dom works.

"react-router-dom": "^6.22.0"

@melloware melloware closed this as not planned Won't fix, can't repro, duplicate, stale Feb 12, 2024
@melloware melloware added Resolution: Invalid Issue or pull request is not valid in the latest version and removed Status: Needs Triage Issue will be reviewed by Core Team and a relevant label will be added as soon as possible labels Feb 12, 2024
@gayathrirajendran
Copy link
Author

gayathrirajendran commented Feb 12, 2024

@melloware A request to check the analysis again, because the issue does not occur with MegaMenu and only with Menu. So it seems specific to Primereact's menu and I leave it up to you.
https://codesandbox.io/p/github/gayathrirajendran/test-menu-route/master?file=%2Fsrc%2FSample.tsx%3A41%2C10&workspaceId=fd406fb7-7b5b-42b3-88a0-e0f2d0a8541f

@melloware
Copy link
Member

In the example you send it was raw <Link> code which is just react router dom. Your example didn't event have a menu. Code sandbox sucks make sure you saved the right version

I suggest using Stackblitz.

@gayathrirajendran
Copy link
Author

@melloware Here is the stackblitz link.
https://stackblitz.com/github/gayathrirajendran/test-menu-route

Menu was/is used in the codesandbox example I sent too, just that the themes added.

@melloware melloware removed the Resolution: Invalid Issue or pull request is not valid in the latest version label Feb 13, 2024
@melloware melloware self-assigned this Feb 13, 2024
@melloware melloware reopened this Feb 13, 2024
@github-actions github-actions bot added the Status: Needs Triage Issue will be reviewed by Core Team and a relevant label will be added as soon as possible label Feb 13, 2024
@melloware
Copy link
Member

Thanks let me take a look.

@melloware melloware added Type: Bug Issue contains a defect related to a specific component. and removed Status: Needs Triage Issue will be reviewed by Core Team and a relevant label will be added as soon as possible labels Feb 13, 2024
@melloware
Copy link
Member

@gayathrirajendran the issue is this fix: #5785

You have both a URL AND a COMMAND on your menu item so its triggering both.

I fixed your sandbox: https://stackblitz.com/edit/github-238lms?file=src%2FSample.tsx

  const items2 = items.map((item) => ({
    ...item,
    command: () => nav(item.url),
    url: null,
  }));

Basically nulling out the url since you are calling nav on the command you don't need the URL to also navigate.

melloware added a commit to melloware/primereact that referenced this issue Feb 13, 2024
@melloware melloware added this to the 10.5.1 milestone Feb 13, 2024
@melloware melloware added the Resolution: Workaround Issue or pull request contains a workaround. It needs to be reviewed further by Core Team label Feb 13, 2024
@melloware
Copy link
Member

Another thing you can do is in your command do event.stopPropagation to prevent the click from bubbling up to the anchor <a> link which is triggering the URL. Up to you.

@gayathrirajendran
Copy link
Author

gayathrirajendran commented Feb 13, 2024

@melloware I need the url for the anchor tag to point to the right href and achieve accessibility compliance. In the example, adding the url without the command refreshes the page. And adding just the command without url renders anchor tag with a blunt href. In the example
https://stackblitz.com/edit/github-238lms-1rtqaf?file=src%2FSample.tsx, have added event.stopPropogation() and event.preventDefault() and that isnt fixing the issue.

@gayathrirajendran
Copy link
Author

Will wait for the fix to be published @melloware

@melloware
Copy link
Member

No there is no fix. The current behavior is the correct behavior. If you have a command and a URL it executes the command first and then the URL.

I just made all the other menus have the same behavior. You have two choices.

Remove url and just use command since you are doing a navigation.

Or in your command method add event.preventDefault();event.stopPropagation(); .

The code is doing the correct thing you are passing the wrong thing to the menu.

@gayathrirajendran
Copy link
Author

gayathrirajendran commented Feb 13, 2024

@melloware

Solution 1: Adding only the url refreshes the entire page. Achieves a11y but cannot use as it refreshes the page.
Solution 2: Adding url and command with preventDefault and stopPropogation, also refreshes the page.
Solution 3: Adding only command. Does not meet a11y.

Can you check the example for one last time https://stackblitz.com/edit/github-238lms-1rtqaf?file=src%2FSample.tsx ? Lets leave it here if you say its intended behavior.

@melloware
Copy link
Member

melloware commented Feb 13, 2024

@gayathrirajendran works totally fine: https://stackblitz.com/edit/github-238lms?file=src%2FSample.tsx

You were doing it incorrectly again. The command receives:

/**
 * Custom command event.
 * @see {@link MenuItem.command}
 * @event
 */
interface MenuItemCommandEvent {
    /**
     * Browser event
     */
    originalEvent: React.SyntheticEvent;
    /**
     * Selected item instance.
     */
    item: MenuItem;
}

So..

  const items2 = items.map((item) => ({
    ...item,
    command: (options: MenuItemCommandEvent) => {
      const originalEvent = options.originalEvent;
      originalEvent.preventDefault();
      originalEvent.stopPropagation();
      nav(item.url);
    },
  }));

@gayathrirajendran
Copy link
Author

Thanks a lot @melloware and sorry for missing the event interface properly... Will take care the next time!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Resolution: Workaround Issue or pull request contains a workaround. It needs to be reviewed further by Core Team Type: Bug Issue contains a defect related to a specific component.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants