-
Notifications
You must be signed in to change notification settings - Fork 91
feat(AppMain): improved banners, section layout, settings shortcut #19670
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
base: master
Are you sure you want to change the base?
Conversation
782584e to
00c176d
Compare
| duration: 500 | ||
| duration: ThemeUtils.AnimationDuration.Slow | ||
| easing.type: Easing.OutCubic | ||
| alwaysRunToEnd: 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.
Do not leave ghost blank space if the animation gets interrupted
| } | ||
|
|
||
| // divider | ||
| Rectangle { |
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.
Internal "divider" between banners
| item.hide() | ||
| return | ||
| } | ||
|
|
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 logic to show banners, based on relevance and/or online status
| id: headerBackgroundSlot | ||
| anchors.top: parent.top | ||
| // Needed cause I see a gap otherwise | ||
| anchors.topMargin: -3 |
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.
Gaps gone but this wasn't the reason :)
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.
Also noticed @Khushboo-dev-cpp already fixed the gap at the top, thanks 👍
| anchors.top: statusToolBar.bottom | ||
| anchors.bottom: footerSlot.top | ||
| anchors.bottomMargin: footerSlot.visible ? 8 : 0 | ||
| anchors.bottomMargin: !!root.footer ? Theme.halfPadding : 0 |
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.
Only add the space/gap if the footer is present
Jenkins BuildsClick to see older builds (33)
|
00c176d to
ea9d292
Compare
jrainville
left a comment
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.
Nice!
- display the banners in appropriate context, and enable them on mobile - if (truly) offline, only display the offline banner (no 3rd party service would work at this point anyway) - fix the blank space that would sometimes be left behind closed banners, and fix the banners spacing (the spacer is moved to inside the banner, otherwise spaces between hidden banners would accumulate and create another blank gap) - fix date/time formatting in the banners - enable the Settings shortcut to work across all relevant sections - some minor fixes & cleanups Fixes #12938 Fixes #18338 Fixes #19298 Fixes #19279
ea9d292 to
1159439
Compare
What does the PR do
Reduces visual clutter with banners
Fixes #12938
Fixes #18338
Fixes #19298
Fixes #19279
Affected areas
AppMain
Architecture compliance
My PR is consistent with this document: QML Architecture Guidelines
Screencapture of the functionality
One banner:

No relevant banners:

Gap in the section layout; before:

After:

Impact on end user
Less distracting banners
How to test
Risk
low