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

Deleted menus reappear when switching to Menu Locations tab and back again #22340

Closed
talldan opened this issue May 14, 2020 · 10 comments
Closed
Assignees
Labels
[Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended

Comments

@talldan
Copy link
Contributor

talldan commented May 14, 2020

Describe the bug
On the experimental navigation menu page, deleted menus reappear when switching to Menu Locations tab and back again.

To reproduce
Steps to reproduce the behavior:

  1. Add some menus
  2. Delete one
  3. Select 'Menu Locations'
  4. Go back to 'Edit Navigation'
  5. Note that the deleted menu is back in the Select Navigation menu.

Expected behavior
The deleted menu doesn't reappear

@talldan talldan added [Type] Bug An existing feature does not function as intended [Block] Navigation Affects the Navigation Block labels May 14, 2020
@talldan
Copy link
Contributor Author

talldan commented May 14, 2020

I believe this is because the stateMenus react state is reset when the component is unmounted and remounted:
https://github.com/WordPress/gutenberg/blob/master/packages/edit-navigation/src/components/menus-editor/index.js#L18

It might need to be hoisted up a few levels.

@draganescu
Copy link
Contributor

I thought I solved that :D Will take a look!

@draganescu draganescu self-assigned this May 14, 2020
@draganescu
Copy link
Contributor

The real problem is

const menus = useSelect( ( select ) => select( 'core' ).getMenus() );

this should know we deleted something and get a fresh copy ✅

It doesn't so we'll probably, for now, need to move the state out of the menusEditor component.

@talldan
Copy link
Contributor Author

talldan commented May 14, 2020

I also thought it might be good if instead of creating stateMenus that has the filtered out deleted menus, to instead retain a list of deletedMenus that can be used to filter out the menus.

At the moment it feels a little fragile as it relies on menus not repopulating.

The psuedocode would be something like:

const menus = useSelect( ( select ) => omit( select( 'core' ).getMenus(), deletedMenus ), [ deletedMenus ] );

@draganescu
Copy link
Contributor

🍾 @talldan I think your idea might work for #21282 as well!

@adamziel
Copy link
Contributor

adamziel commented May 15, 2020

It's a good workaround for the purposes of this experiment. In the longer run we should be addressing the root cause of the issue as in why are these items not getting removed from the store?

@talldan
Copy link
Contributor Author

talldan commented Jun 3, 2020

#22625 looks like the same sort of issue, but relates to menu selection.

@noisysocks
Copy link
Member

noisysocks commented Aug 20, 2020

What's the status of this? @draganescu: Are you still working on it?

It's a good workaround for the purposes of this experiment. In the longer run we should be addressing the root cause of the issue as in why are these items not getting removed from the store?

Agreed.

@draganescu
Copy link
Contributor

I don't think this happens anymore since merging #22428. I tested and it worked. Can anyone else confirm?

@noisysocks
Copy link
Member

I wasn't able to reproduce it either.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants