Add Test Suit for Sidebar Icon and Text Visibility #9588
Conversation
WalkthroughThis pull request focuses on enhancing the Cypress testing for the facility homepage, specifically by adding sidebar functionality tests. The changes introduce a new constant Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
✅ Deploy Preview for care-ohc ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for care-ohc ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/components/Common/Sidebar/SidebarItem.tsx (1)
70-70: Consider sanitizingprops.textor using a slug if it contains special characters.
While this approach is perfectly valid for simple labels, ifprops.textincludes spaces, punctuation, or special characters, it can lead to awkwarddata-testidattributes. Either replace spaces with a “-” or sanitize the string to ensure stable test IDs.cypress/pageobject/Facility/FacilityHome.ts (2)
4-4: Use a more descriptive naming convention for the selector.
Althoughsidebar_toggleis clear, consider a more descriptive naming if this element is specific to toggling shrink/expansion, such assidebarToggleShrinkor similar. This can help maintain clarity in larger projects.
7-9: Correct the function name spelling.
The method nameverifyIconVisiblityappears to contain a spelling error (“Visiblity”). Changing it toverifyIconVisibilitywould improve readability and consistency.- verifyIconVisiblity(iconClassName: string) { + verifyIconVisibility(iconClassName: string) { cy.get(`.${iconClassName}`).should("be.visible").should("exist"); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cypress/e2e/facility_spec/FacilityHomepage.cy.ts(2 hunks)cypress/pageobject/Facility/FacilityHome.ts(1 hunks)src/components/Common/Sidebar/Sidebar.tsx(1 hunks)src/components/Common/Sidebar/SidebarItem.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/components/Common/Sidebar/Sidebar.tsx
🔇 Additional comments (4)
cypress/pageobject/Facility/FacilityHome.ts (2)
11-23: Validate text matching behavior in verifyTextVisibility.
Currently, the method checks for an exact text match with expectedText. This works well if you want precise matching, but be aware that i18n dynamic text or trailing spaces can cause false negatives. Consider partial matches or a different approach if your text might change slightly (e.g., adding punctuation).
25-29: Good approach for toggling.
This method succinctly ensures the toggle element is visible before clicking, which helps avoid flaky tests. No issues spotted here.
cypress/e2e/facility_spec/FacilityHomepage.cy.ts (2)
35-44: NavItems array structure looks good.
Enumerating the text and icon pair in a central list is a nice pattern, maintaining clarity and facilitating updates.
58-76: Well-structured test scenario for the sidebar toggle.
The test thoroughly ensures icons and labels are correctly shown or hidden when toggling. The usage of verifyIconVisiblity and verifyTextVisibility is clear and consistent. Great coverage!
…of github.com:JavidSumra/care_fe into issues/9043/refactor-facility-export-add-sidebar-test
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
cypress/pageobject/Facility/FacilityHome.ts (3)
4-4: Consider maintaining consistent selector patternsThe
sidebar_toggleselector uses a data-testid value directly, while other selectors in the class use CSS/ID selectors (e.g.,exportButton = "#export-button"). Consider updating for consistency:- sidebar_toggle = "sidebar-toggle"; + sidebar_toggle = "[data-testid='sidebar-toggle']";This would also eliminate the need to wrap the selector in
data-testidattribute syntax in thetoggleSideBarmethod.
25-29: Consider handling UI transitionsThe toggle method should account for potential UI transitions or animations that might affect the sidebar's interactivity.
toggleSideBar() { cy.get(`[data-testid="${this.sidebar_toggle}"]`) .should("be.visible") + .should("not.be.disabled") + .should("have.css", "pointer-events", "auto") .click(); + // Wait for any transitions to complete + cy.wait(500); // Adjust based on your transition duration }
4-29: Comprehensive test coverage for sidebar functionalityThe implementation provides good coverage for testing sidebar behavior, including icon visibility, text visibility, and toggle functionality. The page object pattern is well-maintained, making the tests readable and maintainable.
Consider adding the following test scenarios to ensure complete coverage:
- Test for proper aria attributes during sidebar state changes
- Verify sidebar state persistence across page reloads
- Test keyboard navigation accessibility
Would you like me to provide example test cases for these additional scenarios?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cypress/e2e/facility_spec/FacilityHomepage.cy.ts(2 hunks)cypress/pageobject/Facility/FacilityHome.ts(1 hunks)src/components/Common/Sidebar/SidebarItem.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/components/Common/Sidebar/SidebarItem.tsx
- cypress/e2e/facility_spec/FacilityHomepage.cy.ts
|
@nihal467 needs your review |
|
We just migrated to Shadcn's sidebar on |
|
👋 Hi, @JavidSumra, This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
|
No longer relevant, closing. |
Proposed Changes
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
New Features
Bug Fixes
data-testidattributes on sidebar components.Tests