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

Cleanup navigation related code #1037

Open
tomalec opened this issue Oct 5, 2021 · 0 comments
Open

Cleanup navigation related code #1037

tomalec opened this issue Oct 5, 2021 · 0 comments
Labels
category: refactor The issue/PR is related to refactoring. type: enhancement The issue is a request for an enhancement. type: technical debt - external devX This issue/PR suffers from the ergonomics of external tools/dependencies type: technical debt This issue/PR represents/solves the technical debt of the project.

Comments

@tomalec
Copy link
Member

tomalec commented Oct 5, 2021

Describe the bug:

Currently, navigation-related code is scattered around our codebase in multiple places. It makes it hard to add new pages or change the navigation flow. Some of it is a result of Woo convention of adding pages - separate setup in PHP separate in JS. But I believe some of it could be cleaned up on our end.

Here is the list of places when we define the navigation:

  1. All /src/Menu/*.php
  2. /js/src/index.js
  3. /js/src/utils/urls.js - List of internal URLs to be used in links, buttons, etc.
  4. /js/src/components/navigation-classic/main-tab-nav.js
    • a workaround to highlight "current" page in the legacy WC menu
    • list of main tabs

Thoughts, doubts:

  • I'm not entirely sure, what is the relation between 1. (/src/Menu/*.php) and 2. (/js/src/index.js), and if the 2. will be needed in the new nav. Also, why it cannot be built from a single source.

  • I have a gut feeling that 2. (/js/src/index.js) and 3. (/js/src/utils/urls.js) could somewhat share the config.

  • I think, the hack for legacy nav highlighting could be move from 4. /js/src/components/navigation-classic/main-tab-nav.js somewhere else, as it's easy to miss it. (Current, settings subpages & account-reconnection, do not use it). Also, we could use a somewhat more official window.wpNavMenuClassChange instead of manually hacking CSS selectors and classes.
    (Personally, I spend a week working with navigation. Then needed an entire day of debugging the WC-admin navigation to find it.)

@tomalec tomalec added type: enhancement The issue is a request for an enhancement. type: technical debt This issue/PR represents/solves the technical debt of the project. category: refactor The issue/PR is related to refactoring. type: technical debt - external devX This issue/PR suffers from the ergonomics of external tools/dependencies labels Oct 5, 2021
tomalec added a commit that referenced this issue Oct 5, 2021
from MainTabNav.

Simpligy it to use `window.wpNavMenuClassChange` instead of custom CSS selectors hacking.

Addresses part of #1037.
tomalec added a commit that referenced this issue Oct 5, 2021
from `<MainTabNav>`.

Simplify it to use `window.wpNavMenuClassChange` instead of custom CSS selectors hacking.
Use it in `Settings` sub-pages.

Addresses part of #1037.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: refactor The issue/PR is related to refactoring. type: enhancement The issue is a request for an enhancement. type: technical debt - external devX This issue/PR suffers from the ergonomics of external tools/dependencies type: technical debt This issue/PR represents/solves the technical debt of the project.
Projects
None yet
Development

No branches or pull requests

1 participant