Skip to content

Conversation

@sroy3
Copy link
Contributor

@sroy3 sroy3 commented Jul 19, 2022

First 2 tasks of #1604

Screen.Recording.2022-07-19.at.1.31.44.PM.mov

@sroy3 sroy3 added the product PR that affects product label Jul 19, 2022
@sroy3 sroy3 self-assigned this Jul 19, 2022
@sroy3 sroy3 marked this pull request as ready for review July 21, 2022 15:44
@@ -0,0 +1,3 @@
export const idToNodeNode = (id: string) =>
// eslint-disable-next-line unicorn/prefer-query-selector
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is needed to pass Windows tests. When not using getElementById we need a CSS.escape because of all the funky ids we have in both the experiments table and plots. CSS.escape does not seem to create a valid id query on Windows unfortunately.

@sroy3 sroy3 changed the title Comparison table DnD feedback Comparison table drag and drop feedback Jul 21, 2022
@sroy3 sroy3 requested a review from shcheklein July 21, 2022 20:36
Copy link
Contributor

@julieg18 julieg18 left a comment

Choose a reason for hiding this comment

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

Looks good to me!

dragAndDrop(
headerB,
// eslint-disable-next-line testing-library/no-node-access
headerD.parentElement?.parentElement || headerD,
Copy link
Contributor

Choose a reason for hiding this comment

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

[Q] Can we follow up and wrap these calls in a function so we don't have eslint-disable all through the tests?

// eslint-disable-next-line testing-library/no-node-access
      headerD.parentElement?.parentElement || headerD

Copy link
Contributor

Choose a reason for hiding this comment

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

limit them to a single place like we try to do when using any

@sroy3 sroy3 enabled auto-merge (squash) July 22, 2022 13:25
@qlty-cloud-legacy
Copy link

Code Climate has analyzed commit 0480689 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (85% is the threshold).

This pull request will bring the total coverage in the repository to 96.7% (0.0% change).

View more on Code Climate.

@sroy3 sroy3 merged commit 2922378 into main Jul 22, 2022
@sroy3 sroy3 deleted the comparison-d-n-d-feddback branch July 22, 2022 13:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

product PR that affects product

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants