-
Notifications
You must be signed in to change notification settings - Fork 15
Add chevron collapsible table #3598
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
base: main
Are you sure you want to change the base?
Conversation
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.
Hey @arbulu89 great job, it looks good to me only check out my comment about the selectors.
Quick question: why did you go back to using just the chevron? Would it make sense to keep both — the chevron to indicate the table is collapsible, and also keep the row clickable to expand it?
I’m just thinking in terms of usability — making it easier for users to interact with either element but sure @jagabomb had his reasons :D
@@ -117,9 +117,11 @@ export const cleanUpButtonIsNotDisplayed = () => { | |||
|
|||
// UI Interactions | |||
|
|||
export const clickHdqDatabaseRow = () => cy.get(hdqDatabaseCell).click(); | |||
export const clickHdqDatabaseRow = () => | |||
cy.get(`${hdqDatabaseCell} > td:eq(0)`).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.
Hey could we extract this as own selector ?
Someting like this ?
export const hdqDatabaseRowFirstCell = `${hdqDatabaseCell} > td:eq(0)`;
export const hddDatabaseRowFirstCell = `${hddDatabaseCell} > td:eq(0)`;
And put the selectors on top of the file so we have code like this
export const clickHdqDatabaseRow = () => cy.get(hdqDatabaseRowFirstCell).click();
export const clickHddDatabaseRow = () => cy.get(hddDatabaseRowFirstCell).click();
We use this approach through the different page objects.
const hddDatabaseCell = `tr:contains("${hddDatabase.sid}")`; |
const providerLabel = 'div[class*="text-lg"]:contains("Provider") + div'; |
Checkout the adr from @vicenteqa https://github.com/trento-project/docs/blob/main/adr/0019-e2e-testing-practices.md#decisions
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.
Yes, good idea!
Done!
Minor feedback, can we change the padding-left of the arrow/chevron icon to 1em? I think its pl-4 😅. I think it will create a better balance in the row. |
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.
LGTM
Description
Update Table collapsible rows usage to add a chevron to collapse/expand instead of clicking anywhere in the row.
When a table has collapsible rows, the first cell of the row is reserved to add chevron.
How was this tested?
UT and e2e