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

removed the single submenus as per issue:OGC-17 #1354

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

vsvishalsharma
Copy link

@vsvishalsharma vsvishalsharma commented Dec 21, 2024

Pull Request Requirements

  • The PR title includes a brief description of the work done, including the Issue number if applicable.
  • The PR includes a video showing the changes for the work done.
  • The PR title follows conventional commit label standards.
  • The changes conform to the OpenElis Global x3 Styleguide and design documentation.
  • The changes include tests or are validated by existing tests.
  • I have read and agree to the Contributing Guidelines of this project.

Summary

This PR resolves issue OGC-17 by improving the navigation menu functionality. It ensures that if a menu has only one submenu, the submenu is replaced with a direct link, using the parent menu's name as the navigation label. This enhancement simplifies user interaction by reducing unnecessary nesting in the navigation structure.

Screenshots

Before: Submenu were displayed even when there was only one submenu.

Screenshot from 2024-12-20 23-04-53

After: Direct link is shown for menus with a single submenu.

Screenshot from 2024-12-21 16-47-13

Related Issue

OGC-17

#1359

@vsvishalsharma
Copy link
Author

All the nav links are working as expected

Screencast.from.21-12-24.05.15.14.PM.IST.webm

Copy link
Contributor

@Bahati308 Bahati308 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hello @vsvishalsharma , did you run this command cd frontend and then npm run format ??

@vsvishalsharma
Copy link
Author

@Bahati308 thanks for mentioning it !

@vsvishalsharma
Copy link
Author

vsvishalsharma commented Dec 24, 2024

@caseyi ,@mozzy11 i need your feedback on

Issue:

Current test flows expect dropdown navigation for Pathology/Immunochemistry menus, but we simplified UX for single-submenu cases to use direct links. This breaks existing tests.

impact:

#menu_pathology_dropdown clicks failing in cypress test for dashboard.cy.js
Status selection errors
Similar issues in Immunochemistry flow

Options:

Update test flows to match new UX
Revert to dropdown pattern with auto-expansion

I can implement either solution and am willing to consider any other approach based on your feedback!

@adityadeshlahre
Copy link
Contributor

@caseyi ,@mozzy11 i need your feedback on

Issue:

Current test flows expect dropdown navigation for Pathology/Immunochemistry menus, but we simplified UX for single-submenu cases to use direct links. This breaks existing tests.

impact:

#menu_pathology_dropdown clicks failing in cypress test for dashboard.cy.js Status selection errors Similar issues in Immunochemistry flow

Options:

Update test flows to match new UX Revert to dropdown pattern with auto-expansion

I can implement either solution and am willing to consider any other approach based on your feedback!

@vsvishalsharma Option 1 is definitely a way to go just modify dashboard.cy.js file and test it locally and make sure the test is passing locally on a fresh instance.

@Bahati308
Copy link
Contributor

@Bahati308 thanks for mentioning it !

welcome. Did you run it, you need to pass the checks locally. In case of any challenge, reach out

@vsvishalsharma vsvishalsharma force-pushed the menu_changes branch 3 times, most recently from 1184a2f to 0242577 Compare December 25, 2024 06:29
@vsvishalsharma
Copy link
Author

@adityadeshlahre if possible can you run the ci on this

@adityadeshlahre
Copy link
Contributor

@adityadeshlahre if possible can you run the ci on this

image

seems like all cypress tests are passing at least in headless mode 🙂

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