Skip to content
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

frontend: TopBar: Hide menu on mobile when empty #2603

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

skoeva
Copy link
Contributor

@skoeva skoeva commented Nov 22, 2024

This change hides the top bar menu on mobile when there are no actions to display.

Fixes: #2066

Testing

  • Open Headlamp in the mobile view
  • Ensure that the top bar menu shows up only when there are available actions

image

@skoeva skoeva added frontend Issues related to the frontend mobile Issues affecting small screens labels Nov 22, 2024
@skoeva skoeva self-assigned this Nov 22, 2024
@skoeva skoeva linked an issue Nov 22, 2024 that may be closed by this pull request
@skoeva skoeva force-pushed the fix-broken-menu branch 2 times, most recently from f0d48a0 to 527a1dd Compare November 27, 2024 17:41
@skoeva skoeva marked this pull request as ready for review November 27, 2024 19:03
@dosubot dosubot bot added the size:M This PR changes 30-99 lines, ignoring generated files. label Nov 27, 2024
Copy link
Contributor

@sniok sniok left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

image

image

frontend/src/components/App/TopBar.tsx Outdated Show resolved Hide resolved
@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:M This PR changes 30-99 lines, ignoring generated files. labels Dec 2, 2024
@skoeva
Copy link
Contributor Author

skoeva commented Dec 4, 2024

Tested by enabling/disabling the dynamic-clusters plugin, and works as expected:

  • When the plugin is enabled, the menu shows up with the dynamic cluster button

image

  • When the plugin is disabled, the menu does not show up

image

Copy link
Collaborator

@joaquimrocha joaquimrocha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

frontend/src/components/App/TopBar.tsx Outdated Show resolved Hide resolved
@sniok
Copy link
Contributor

sniok commented Dec 9, 2024

I am a bit puzzled with this one, since it looks like we are changing the functionality that plugins may be overriding

Yeah so my request was to change how the buttons look on mobile (icon with a label).
And now it does that and looks a lot better but at the same time breaking compatibility with plugins by introducing a new separate properties to AppBarAction.
And we can't really do that, we need to keep rendering only .action component.

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?

@skoeva
Copy link
Contributor Author

skoeva commented Dec 11, 2024

@sniok Sounds good. Will move the additional changes into a new PR and adjust this one accordingly

@skoeva skoeva closed this Dec 16, 2024
@dosubot dosubot bot added size:XS This PR changes 0-9 lines, ignoring generated files. and removed size:L This PR changes 100-499 lines, ignoring generated files. labels Dec 16, 2024
This change hides the top bar menu on mobile when there are no actions
to display.

Fixes: #2066

Signed-off-by: Evangelos Skopelitis <[email protected]>
@skoeva skoeva reopened this Dec 16, 2024
@dosubot dosubot bot added size:S This PR changes 10-29 lines, ignoring generated files. and removed size:XS This PR changes 0-9 lines, ignoring generated files. labels Dec 16, 2024
@skoeva skoeva changed the title frontend: Improve top bar menu on mobile frontend: TopBar: Hide menu on mobile when empty Dec 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
frontend Issues related to the frontend mobile Issues affecting small screens size:S This PR changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Empty menu on home page on mobile viewport
3 participants