-
Notifications
You must be signed in to change notification settings - Fork 157
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
chore: Add regression tests for runtime preferences handling in multi-app-layout setup #3045
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3045 +/- ##
=======================================
Coverage 96.34% 96.34%
=======================================
Files 780 780
Lines 21923 21927 +4
Branches 7502 7502
=======================================
+ Hits 21121 21126 +5
- Misses 750 794 +44
+ Partials 52 7 -45 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
@@ -55,6 +55,7 @@ export default function () { | |||
return ( | |||
<ScreenshotArea gutters={false}> | |||
<AppLayout | |||
{...{ __disableRuntimeDrawers: true }} |
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.
Moved this here to more accurately represent real-world use-case
type: 'global', | ||
defaultActive: false, | ||
resizable: true, | ||
defaultSize: 320, | ||
|
||
ariaLabels: { | ||
closeButton: 'Close button', | ||
content: 'Content', | ||
content: 'Drawer with counter', |
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.
Improvement for manual testing. All drawers have the same circle icons, let's have different tooltips at least
@@ -53,7 +55,6 @@ describe.each(['classic', 'refresh', 'refresh-toolbar'] as Theme[])('%s', theme | |||
test( | |||
'renders according to defaultSize property', | |||
setupTest({}, async page => { | |||
await page.setWindowSize(viewports.desktop); |
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.
Removed duplicated lines, this is in the shared setup
return; | ||
} | ||
const document = panelElement.ownerDocument; |
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 really a functional change, but in our demo page setup the document
points on the wrong frame and we need to account for that
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.
Minor: shadowing the document
variable rather than just calling it panelDocument
could cause issues when we debug or refactor this code.
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 did it this way to make the final diff shorter, but I can also rename if you find it more important
fa9a2f4
to
623c9cf
Compare
623c9cf
to
afe2bf2
Compare
@@ -76,10 +76,38 @@ describe.each(['classic', 'refresh', 'refresh-toolbar'] as Theme[])('%s', theme | |||
}) | |||
); | |||
|
|||
test( | |||
'should persist runtime drawer preferences when switching between multiple app layouts', |
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.
All changes were for the sake of writing this test
a7dc895
to
2df6dbd
Compare
return; | ||
} | ||
const document = panelElement.ownerDocument; |
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.
Minor: shadowing the document
variable rather than just calling it panelDocument
could cause issues when we debug or refactor this code.
…-app-layout setup
2df6dbd
to
4f9cd75
Compare
Description
More regression tests for this PR #3022
Related links, issue #, if available: n/a
How has this been tested?
Test changes only
Review checklist
The following items are to be evaluated by the author(s) and the reviewer(s).
Correctness
CONTRIBUTING.md
.CONTRIBUTING.md
.Security
checkSafeUrl
function.Testing
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.