-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Migrate Sankey renderer to React #4255
Conversation
8e74f1a
to
41cba71
Compare
const links = {}; | ||
const nodes = []; | ||
|
||
// ANGULAR_REMOVE_ME $$ check is for Angular's internal properties |
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.
Not redundant now?
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.
No - sankey may still receive data from Angular part, so these properties may be present there. It will be safe to remove when we'll completely remove Angular itself.
|
||
// wait a bit before taking snapshot | ||
cy.wait(1000); // eslint-disable-line cypress/no-unnecessary-waiting | ||
cy.percySnapshot('Visualizations - Sunburst', { widths: [viewportWidth] }); |
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.
Perhaps you can skip the wait by instead of clicking "Show Data Only", visiting page cy.visit('queries/${id}')
and then snap?
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 added this timeout to wait for chart to load a query results data. "Show Data Only" is actually a link, so it should be completely equal to cy.visit(...)
. But I may give it a try
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.
Seems there is no difference - cy.visit
need cy.wait
as well. I prefer to keep current variant because 1) it already works 2) it's a bit simpler 🙂
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.
Looks good. A few comments.
What type of PR is this? (check all applicable)
Description
Related Tickets & Documents
#3301 (Migrate Visualizations to React -> Sankey)
Mobile & Desktop Screenshots/Recordings (if there are UI changes)
No visual changes