-
-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
fix(Control): calcCornerCoords
angle + calculation
#9377
Conversation
Build Stats
|
7d1d111
to
39f4b7e
Compare
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.
ready
If you want this to become more than one PR say so
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.
updated desc
validated the snapshot against master so all is good to go
const { calls, results } = spies[index].mock; | ||
return Object.assign(acc, { [key]: { calls, results } }); | ||
}, {}) | ||
).toMatchSnapshot(roundSnapshotOptions); |
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.
Very ugly snapshot, the playwright test should be enough to ensure coords take angle correctly into account. Debugging a diff change in this snapshot is a nightmare and pointless
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.
Is better to have tests that test one assertion, a snapshot covers a lot of things but doesn't tell you want wants to preserver. I changed the test to measure the angle of the corners
@ShaMan123 #8767 is mentioned to corroborate any change. |
The benchmark show that this approach other than terser is also a bit faster. |
i need to look at the jest test, but i m too tired now, i ll do that first thing tomorrow |
Motivation
Description
The angle passed to
calcCornerCoords
should be the angle from the viewport in order to respect nested objects. Since currently vpt rotation is not supported it is enough to get the angle in the scene plane but that will become a bug once vpt rotation is supported (e.g. #8767 )Additionally the calculation was cumbersome and very hard to follow.
There were no tests for it, I added both unit and e2e tests, e2e to prove that my fix is correct.
Changes
Gist
In Action
Actual is from master, expected is this PR
Playwright.Test.Report.-.Google.Chrome.2023-09-25.13-10-20.-.Trim.mp4