-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
[ui][fix] Application Menubar: Ensure Menubar occupies all of available height of the layout #2643
base: develop
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #2643 +/- ##
========================================
Coverage 69.96% 69.96%
========================================
Files 121 121
Lines 7084 7084
========================================
Hits 4956 4956
Misses 2128 2128 ☔ View full report in Codecov by Sentry. |
The main issue I have with this change is that, although the menu bar now takes up the whole space as it should, the menus themselves are not vertically centered with the homepage icon. The menu bar starts slightly higher than the homepage button but finishes slightly higher as well, and it's very clear when hovering. It'd be great if the button and the menu bar could be aligned. |
75b8ac2
to
21ed191
Compare
meshroom/ui/qml/Application.qml
Outdated
MenuBar { | ||
palette.window: Qt.darker(activePalette.window, 1.15) | ||
anchors.bottom: parent.bottom | ||
anchors.bottomMargin: 1 /// this margin counters the offset vertically when lining up against with the homeButton |
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 better to me without this offset.
anchors.bottomMargin: 1 /// this margin counters the offset vertically when lining up against with the homeButton |
meshroom/ui/qml/Application.qml
Outdated
|
||
MenuBar { | ||
palette.window: Qt.darker(activePalette.window, 1.15) | ||
anchors.bottom: parent.bottom |
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.
if we could avoid anchors that's ideal.
Rectangle { | ||
/** | ||
* This Rectangle serves as the parent of the menubar | ||
* occupying all of the additional space which the MenuBar is not going to get | ||
*/ | ||
Layout.fillWidth: true | ||
Layout.fillHeight: true | ||
color: Qt.darker(activePalette.window, 1.15) | ||
|
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.
Just using "height: parent.height" on the MenuBar seems to work, without adding a Rectangle into the hierarchy.
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 rectangle already existed to cover up additional space horizontally, but I could try using height property to set this, last I can recall, there was a very minor pixel worth of offset against the Custom buttons. Let me try the changes, thanks.
…e parent layout Reducing the default padding on the Home and Execution buttons reduced the overall height of the header and aligning the Menubar inside a Parent Rect occupying additional space ensures the lineup of the menu against the tool buttons
21ed191
to
5f77674
Compare
Description
This PR modifies the
Menubar
component to ensure it fills the layout in terms of height.The change addresses an inconsistency where the MenuBar did not align with the Parent's dimensions.
Details of the change
The above changes ensure the overall alignment of the Header bar elements, Custom buttons and MenuBar.
Appearence after the changes