-
Notifications
You must be signed in to change notification settings - Fork 354
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
test: datagrid scrolling [INFENG-687] #9379
Conversation
✅ Deploy Preview for determined-ui ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
@@ -52,6 +52,7 @@ test.describe('Experiement List', () => { | |||
const row = await projectDetailsPage.f_experiemntList.dataGrid.getRowByColumnValue('ID', '1'); | |||
await row.clickColumn('Select'); | |||
expect(await row.isSelected()).toBeTruthy(); | |||
await expect((await row.getCellByColumnName('Checkpoints')).pwLocator).toHaveText('0'); |
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.
this would fail if the table isn't scrolled to the row. in the DOM, the column wouldn't exist.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #9379 +/- ##
==========================================
- Coverage 48.12% 42.88% -5.24%
==========================================
Files 1219 909 -310
Lines 157884 118784 -39100
Branches 2731 2731
==========================================
- Hits 75975 50946 -25029
+ Misses 81734 67663 -14071
Partials 175 175
Flags with carried forward coverage won't be shown. Click here to find out more. |
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.
This does seem like a pain. I think it would be helpful if somewhere in the datagrid there was a summary of the algorithm we intend to use for scrolling.
@@ -26,7 +26,7 @@ export class F_ExperiementList extends BaseReactFragment { | |||
readonly pagination = new Pagination({ parent: this }); | |||
} | |||
|
|||
class ExperimentHeadRow extends HeadRow {} | |||
class ExperimentHeadRow extends HeadRow<ExperimentRow, ExperimentHeadRow> {} |
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.
can class ExperimentHeadRow extends HeadRow<ExperimentRow, ExperimentHeadRow>
change to class ExperimentHeadRow extends HeadRow<ExperimentRow>
? Having a class reference itself as a generic seems like a pretty strong code smell. There should be no reason for it.
Since the compiler already has the class type from the parent class here, we shouldn't need to specify it again. I think just removing the second generic class works but I can't pull to try it right now for unknown reasons.
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.
i made it a little better. check again
0, | ||
); | ||
// scrolling isn't waited on, and in this instance it's not guaranteed either. let's just wait a bit. | ||
await page.waitForTimeout(3_000); |
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.
If at all possible we should find something else to wait on here. Maybe we could take a locator to expect to see as a parameter and we could wait visible? Maybe that checkColumnInView function could be passed as a parameter so we could parameterize the wait
function.
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.
well typically we'd want to wait on the columns to change, but the columns wont change if the last column is already in view
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.
yeah the problem is this is too strict checkColumnInView
the best we could do is "wait for columns to change" but even that wouldn't be guaranteed. All we can do is wait for a change to "maybe" happen, and then check with
if (JSON.stringify(indexes) === JSON.stringify(prevIndexes)) {
return false;
}
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.
im just gonna leave this here since i considered it, but at this moment i can't figure out what else I'd wait on anyway.
/**
* Scolls the table
* @param {object} obj
* @param {number} [obj.xAbsolute] - The x coordinate. If not provided, the table will incrementally scroll to the right.
* @param {Function} [obj.waitMethod] - The method we use to wait for the table to scroll. Default is to wait 3 seconds.
*/
private async scrollTable({ xAbsolute, waitMethod = async () => await this.root._page.waitForTimeout(3000) }: { xAbsolute?: number, waitMethod?: () => Promise<void> } = {}): Promise<void> {
const box = await this.pwLocator.boundingBox();
if (box === null) {
throw new Error('Expected to see a bounding box for the table.');
}
const page = this.root._page;
// move mouse to the center of the table
await page.mouse.move((box.x + box.width) / 2, (box.y + box.height) / 2);
// scroll the table
await page.mouse.wheel(
xAbsolute ? xAbsolute : box.width - 450,
0,
);
await waitMethod();
}
await page.mouse.move((box.x + box.width) / 2, (box.y + box.height) / 2); | ||
// scroll the table | ||
await page.mouse.wheel( | ||
'xAbsolute' in args ? args.xAbsolute : Math.max(box.width - args.xRelative, 200), |
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.
The Math for the relative doesn't make sense to me. What is it relative to? The docs make it sound like the default scroll behavior is already relative to current position. It doesn't look like the we can ever scroll back(negative) with relative either.
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.
i just wanted to scroll the table relative to the fixed left side. maybe i should just set increment=true
and scroll ~400px
5f34030
to
06854b6
Compare
06854b6
to
756d302
Compare
c6b18c1
to
e1b615a
Compare
Ticket
INFENG-687
Description
Turns out I needed to scroll the datagrid in order to view it's contents. Check the example in the test case before looking through the datagrid changes.
Test Plan
Checklist
docs/release-notes/
.See Release Note for details.