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

Create Tools menu #1595

Merged

Conversation

rafaellehmkuhl
Copy link
Member

@rafaellehmkuhl rafaellehmkuhl commented Jan 20, 2025

To be merged after #1594.

This PR splits the Settings menu and creates a new Tools menu, with the MAVLink and Data-lake submenus.

It will help a lot with vertical space, since the Settings menu was already bloated.

image

Here are also some screens for before and after with the improvements made in the second commit:

image image

And this is the new one:

image image

@rafaellehmkuhl rafaellehmkuhl marked this pull request as draft January 20, 2025 19:04
@rafaellehmkuhl
Copy link
Member Author

Will put on draft till we merge #1594.

@ES-Alexander ES-Alexander added the docs-needed Change needs to be documented label Jan 22, 2025
src/App.vue Outdated
:icon-class="
interfaceStore.isOnSmallScreen
? 'scale-[100%] -mb-[1px] md:ml-[2px]'
: 'scale-[97%] lg:ml-[1px] -mr-[2px] xl:-mb-[4px]'
Copy link
Contributor

Choose a reason for hiding this comment

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

If you set the scale here to 90% the icon proportion gets better for this icon in particular:

image

@rafaellehmkuhl rafaellehmkuhl force-pushed the create-tools-menu branch 2 times, most recently from a8db7bb to ba2e9c0 Compare January 28, 2025 22:14
@rafaellehmkuhl rafaellehmkuhl marked this pull request as ready for review January 28, 2025 22:14
Copy link
Contributor

@ES-Alexander ES-Alexander left a comment

Choose a reason for hiding this comment

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

The menu itself works as expected, and doesn't seem to have caused any issues with the existing settings menu :-)

That said, the sidebar in general seems to have some weird scaling problems, whereby for a large height region the icon size is determined more by window width than height, and if it gets cut off there's no option to scroll up/down to the missing options:
Screenshot 2025-01-29 at 6 50 23 pm

If necessary we can handle that as a separate issue/PR, but given this menu is being created partly to alleviate the vertical stress from having too many items, I think it makes sense to tackle the scaling issue here too 🤷

@rafaellehmkuhl
Copy link
Member Author

The menu itself works as expected, and doesn't seem to have caused any issues with the existing settings menu :-)

That said, the sidebar in general seems to have some weird scaling problems, whereby for a large height region the icon size is determined more by window width than height, and if it gets cut off there's no option to scroll up/down to the missing options

I've limited the large main-menu to heights above 900 px. Less than that and it uses the simplified version.

I've also fixed the simplified version for the sub-menus (config and tools) as they were using the labels and breaking the layout.

@ES-Alexander
Copy link
Contributor

I've limited the large main-menu to heights above 900 px. Less than that and it uses the simplified version.

That might be a little aggressive - I now can't see the full menu size in a chrome tab on my 16" MacBook, despite there seemingly being room for it, and the small version ends up looking tiny.

Also, there are still apparently 4 different sizes of sidebar determined entirely by the window width (plus a weird About icon)...

Screen.Recording.2025-01-30.at.10.39.37.am.mov

The simplified main menu is shown when the screen height is too small to
show the full menu.

The threshold is different for small and large screens.
@rafaellehmkuhl
Copy link
Member Author

I've limited the large main-menu to heights above 900 px. Less than that and it uses the simplified version.

That might be a little aggressive - I now can't see the full menu size in a chrome tab on my 16" MacBook, despite there seemingly being room for it, and the small version ends up looking tiny.

Also, there are still apparently 4 different sizes of sidebar determined entirely by the window width (plus a weird About icon)...

I think I got to a solution that improves the logic of using the simplified menu a lot.

I don't like it, as it's not future-proof, but it solves the problem for now.

Can you check again?

Copy link
Contributor

@ES-Alexander ES-Alexander left a comment

Choose a reason for hiding this comment

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

This is very unintuitive behaviour, right? I think there's a calculation going wrong somewhere because I can't imagine you intended it to be a tiny menu for a very wide window and then tall for a less wide one.

Screen.Recording.2025-02-04.at.4.34.20.am.mov

Comment on lines +518 to +519
const heightTopBar = widgetStore.currentTopBarHeightPixels * topBottomBarScale.value
const heightBottomBar = widgetStore.currentBottomBarHeightPixels * topBottomBarScale.value
Copy link
Contributor

Choose a reason for hiding this comment

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

Given how the sidebar is changing, I think these might not be being calculated correctly, and I'm still not sure why they're based on windowWidth

@rafaellehmkuhl
Copy link
Member Author

This is very unintuitive behaviour, right? I think there's a calculation going wrong somewhere because I can't imagine you intended it to be a tiny menu for a very wide window and then tall for a less wide one.

I agree it's not perfect, but I think it is way better than what we have today (which is buggy and getting the menu out of the window). I tried to fix the calculations, but it has a lot of deep roots inside the code, and at the moment I don't have much time to work on this.

I suggest two alternatives:

  1. We merge it as it is, with the temporary improvement to the menus calculations
  2. We merge only the first commit

I believe we should at least merge the first commit, because right now the the settings menu is bloated and we have other PRs like #1643 waiting for this change to enter master.

What do you think?

One way or another I opened #1645 to track the final fix for a future PR.

Copy link
Contributor

@ES-Alexander ES-Alexander left a comment

Choose a reason for hiding this comment

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

Ok, let's merge as-is. It's weird, but that can be handled in a subsequent (hopefully quite soon :-P) PR :-)

@rafaellehmkuhl
Copy link
Member Author

For more context, this is the current situation:

image image

And this is the new one:

image image

Both use the same window size, for fair comparisons.
It's important to mention that for very small screens not even the new version is completely visible, and this should be fixed on #1645 as well.

@rafaellehmkuhl rafaellehmkuhl dismissed ArturoManzoli’s stale review February 3, 2025 18:14

Eliot made a further review and approved the PR.

@rafaellehmkuhl rafaellehmkuhl merged commit 4b192a6 into bluerobotics:master Feb 3, 2025
11 checks passed
@rafaellehmkuhl rafaellehmkuhl deleted the create-tools-menu branch February 3, 2025 18:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs-needed Change needs to be documented
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants