-
Notifications
You must be signed in to change notification settings - Fork 522
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
Refactored the pagination component in cypress #9163
Changes from 5 commits
c245e57
c9fc474
052b4a4
f533a07
f76a024
2fb5503
bddb37d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,19 +0,0 @@ | ||
export class AssetPagination { | ||
navigateToNextPage() { | ||
// only works for desktop mode | ||
cy.get("button#next-pages").click(); | ||
} | ||
|
||
verifyNextUrl() { | ||
cy.url().should("include", "page=2"); | ||
} | ||
|
||
navigateToPreviousPage() { | ||
// only works for desktop mode | ||
cy.get("button#prev-pages").click(); | ||
} | ||
|
||
verifyPreviousUrl() { | ||
cy.url().should("include", "page=1"); | ||
} | ||
} | ||
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
export const pageNavigation = { | ||
navigateToNextPage() { | ||
cy.get("button#next-pages").click(); | ||
}, | ||
Comment on lines
+2
to
+4
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Enhance navigation methods with better error handling and assertions. The current implementation might be fragile in real-world scenarios where buttons may be disabled or not immediately available. Consider these improvements: navigateToNextPage() {
- cy.get("button#next-pages").click();
+ cy.get("button#next-pages")
+ .should("be.visible")
+ .should("not.be.disabled")
+ .click();
},
navigateToPreviousPage() {
- cy.get("button#prev-pages").click();
+ cy.get("button#prev-pages")
+ .should("be.visible")
+ .should("not.be.disabled")
+ .click();
}, Also applies to: 10-12 |
||
|
||
verifyCurrentPageNumber(pageNumber: number) { | ||
cy.url().should("include", `page=${pageNumber}`); | ||
}, | ||
Comment on lines
+6
to
+8
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Strengthen page number verification logic. The current URL verification might miss edge cases or provide false positives. Consider a more robust implementation: verifyCurrentPageNumber(pageNumber: number) {
- cy.url().should("include", `page=${pageNumber}`);
+ cy.url().then((url) => {
+ const params = new URLSearchParams(new URL(url).search);
+ const currentPage = parseInt(params.get("page") || "1", 10);
+ expect(currentPage).to.equal(pageNumber);
+ });
+ // Optionally verify the active state in pagination UI
+ cy.get(`[data-testid="page-${pageNumber}"]`)
+ .should("have.attr", "aria-current", "page");
},
|
||
|
||
navigateToPreviousPage() { | ||
cy.get("button#prev-pages").click(); | ||
}, | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix the import path to use relative notation
The import path should be relative to the current file's location. Update it to use the proper relative path notation.
📝 Committable suggestion