frontend: TopBar: Hide menu on mobile when empty#2603
Conversation
f0d48a0 to
527a1dd
Compare
There was a problem hiding this comment.
Thanks for taking a look at it
I tried it and the menu was always missing, I left a comment on the place that has an issue.
Also I think this PR is a good place to also fix the look of the menu on mobile
It's a bit awkward with just an icon in the middle, I think adding label to buttons and aligning them to the left would look a lot better. Similar to how other popup menus look in the app
527a1dd to
54130f9
Compare
54130f9 to
e02c9c5
Compare
joaquimrocha
left a comment
There was a problem hiding this comment.
I am a bit puzzled with this one, since it looks like we are changing the functionality that plugins may be overriding. @sniok can you check/confirm this?
It at least looks like we should have a couple of commits, since the original issue is a menu that is rendering as empty.
Yeah so my request was to change how the buttons look on mobile (icon with a label). I think a better solution here is to render different buttons depending on which menu it is (desktop/mobile), that way we don't have to introduce any new fields or break compatibility const allAppBarActions = [
{ action: <SettingsButton />, }
// ...
];
const mobileAppBarActions = [
{ action: <SettingsButton variant="mobile" /> }
// ...
]But looking at how the scope of this PR keeps growing how about we limit this PR to the initial fix (not rendering menu when it's empty). And then any additional improvements we can move into a new PR. @skoeva What do you think? |
|
@sniok Sounds good. Will move the additional changes into a new PR and adjust this one accordingly |
e02c9c5 to
c97a07c
Compare
This change hides the top bar menu on mobile when there are no actions to display. Fixes: #2066 Signed-off-by: Evangelos Skopelitis <eskopelitis@microsoft.com>
018e9f7 to
c49aada
Compare




This change hides the top bar menu on mobile when there are no actions to display.
Fixes: #2066
Testing