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

add an extra click in nav test to force nav to rerender #583

Merged
merged 7 commits into from
Aug 2, 2023

Conversation

circlecube
Copy link
Member

Proposed changes

Somehow the nav test started failing. Looking into it clicking on a parent item, doesn't seem to always open/show the sub nav list in the test. Manual testing passes for this for me, when I click around I see the subnav right away (unless I click on the WordPress admin link for these pages, then the subnav doesn't appear). I think it has something to do with this document.click and the SubMenusManager here (https://github.com/bluehost/bluehost-wordpress-plugin/blob/main/src/app/components/app-nav/index.js#L75). I'm still not sure why it works when I click and doesn't when cypress clicks, but might be related to event bubbling up the dom and maybe it doesn't go as far when triggered by cypress.

This adds an extra click and makes the tests pass for now, but we should sort this out. It's likely only a nuisance for a user every once in a while. Maybe we wait and fix in our new UI components? Or if we fix it sooner, we should make sure the fix is incorporated into the new nav components.

Type of Change

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation Update (if none of the other choices apply)

Checklist

  • I have read the CONTRIBUTING doc
  • Linting and tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

Further comments

@circlecube circlecube requested review from wpscholar and wpalani August 2, 2023 20:40
@circlecube circlecube self-assigned this Aug 2, 2023
@replay-io
Copy link

replay-io bot commented Aug 2, 2023

1 replays were recorded for 1cd25e8.

image 1 Failed
  • tests/cypress/integration/navigation.cy.js
    1. Logo Links to home
    2. Admin Subnav properly highlights
    3. Main nav links properly navigates
    4. Subnav links properly navigates
      CypressError: Timed out retrying after 4050ms: `cy.click()` failed because the center of this element is hidden from view:
    5. Utility nav links properly navigates
    6. Mobile nav links dispaly for mobile
    7. Mobile nav links properly navigates
image 0 Passed

View test run on Replay ↗︎

@wpalani
Copy link
Member

wpalani commented Aug 2, 2023

@circlecube I agree, this should be fixed at the component level to address the root cause.

I pushed a commit to address the issue: 6b43434

Currently, the Nav triggers a function to handle sub-navigation state on dom click by tracking the active class. When a route changes, the operation might take some time to complete (switch active element).

With this change, we'll track the click event for operations inside the app as well as tracking route changes.

@wpscholar wpscholar merged commit 72db716 into develop Aug 2, 2023
@wpscholar wpscholar deleted the fix/nav-test branch August 2, 2023 23:17
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.

3 participants