-
Notifications
You must be signed in to change notification settings - Fork 176
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
Modified the global default viewport size for the app back to the original size #10579
Modified the global default viewport size for the app back to the original size #10579
Conversation
Codecov Report
@@ Coverage Diff @@
## edge #10579 +/- ##
==========================================
- Coverage 73.83% 73.82% -0.01%
==========================================
Files 2141 2146 +5
Lines 57638 57944 +306
Branches 5848 5953 +105
==========================================
+ Hits 42555 42780 +225
- Misses 13846 13902 +56
- Partials 1237 1262 +25
Flags with carried forward coverage won't be shown. Click here to find out more.
|
ui: { | ||
...MOCK_CONFIG_V7.ui, | ||
width: 1024, | ||
minWidth: 600, |
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 line is unnecessary as minWidth
from v7 is already set to 600
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.
Thanks for the feedback I have made all of the corrections. Now ready for review.
app-shell/src/config/migrate.ts
Outdated
ui: { | ||
...prevConfig.ui, | ||
width: 1024, | ||
minWidth: 600, |
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.
same comment here about not including the minWidth
in this migration as it's left unchanged
app/src/redux/config/schema-types.ts
Outdated
ui: ConfigV7['ui'] & { | ||
minWidth: number | ||
} |
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 ui
key is unchanged between v7 and v8, so these three lines should be unnecessary
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 to me.
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.
📏 📐
Overview:
I have added V8 configuration details for the default app size to be the original size.
Closes #10513
Changelog:
Edited default app window width and height in V8 config migration
Review requests:
In the app you will see the original default app size.
Risk assessment:
Low