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

core: frontend: menus: Organize menu to new logic #1424

Merged

Conversation

patrickelectric
Copy link
Member

@patrickelectric patrickelectric commented Jan 31, 2023

Two categories: settings and tools

image

Based on SMS feedback

Also align side menu icons

Signed-off-by: Patrick José Pereira patrickelectric@gmail.com

Copy link
Collaborator

@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.

A few page renaming suggestions.

Also, from your screenshot I've just seen that the pirate icons are apparently increasing the indentation for those page names, which is a bit weird. Not sure if that's easily fixed here or if it should be raised as an issue and fixed later.

core/frontend/src/menus.ts Outdated Show resolved Hide resolved
core/frontend/src/menus.ts Outdated Show resolved Hide resolved
core/frontend/src/menus.ts Outdated Show resolved Hide resolved
core/frontend/src/menus.ts Outdated Show resolved Hide resolved
core/frontend/src/menus.ts Outdated Show resolved Hide resolved
core/frontend/src/menus.ts Outdated Show resolved Hide resolved
core/frontend/src/menus.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@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.

Mostly looks good :-)

I didn't look at the descriptions earlier, so there are some minor suggestions for them here.

core/frontend/src/menus.ts Outdated Show resolved Hide resolved
core/frontend/src/menus.ts Outdated Show resolved Hide resolved
core/frontend/src/menus.ts Outdated Show resolved Hide resolved
core/frontend/src/menus.ts Outdated Show resolved Hide resolved
core/frontend/src/menus.ts Outdated Show resolved Hide resolved
core/frontend/src/menus.ts Outdated Show resolved Hide resolved
core/frontend/src/menus.ts Outdated Show resolved Hide resolved
core/frontend/src/menus.ts Outdated Show resolved Hide resolved
@joaoantoniocardoso
Copy link
Collaborator

joaoantoniocardoso commented Feb 2, 2023

image
Can we use this opportunity to align the labels?

... actually, the current master is correctly aligned. Also the font differs... Is your photo an actual screenshot? Is this your browser font only?

image

@patrickelectric
Copy link
Member Author

math-lady

@joaoantoniocardoso
Copy link
Collaborator

math-lady math-lady

see my edit

@patrickelectric
Copy link
Member Author

It appears to be a problem when you enable pirate mode, the pirate icons move the item title.

@patrickelectric
Copy link
Member Author

@joaoantoniocardoso check now

@joaoantoniocardoso
Copy link
Collaborator

joaoantoniocardoso commented Feb 2, 2023

This part seems okay, but the menu name is slightly off compared to the category labels

  1. non pirate
    image
  2. pirate
    image
  3. back to non pirate
    image

@ES-Alexander
Copy link
Collaborator

ES-Alexander commented Feb 2, 2023

Also the font differs... Is your photo an actual screenshot? Is this your browser font only?

Yes, that's an actual screenshot from my system. I was using Orion, but I seemingly have the same font on Chrome. I have not seen the font from your screenshot before, although I do have at least a vague recollection of my system giving "failed to fetch font" warnings previously in the console (or something like that), but I do not have such warnings at the moment.

the menu name is slightly off compared to the category labels

@joaoantoniocardoso I think it's that Dashboard is a page, which seemingly has a smaller left pad or margin or something than the dropdown sections. You'll note that the icon is also a bit further to the left for Dashboard than Tools, so the adjustment to fix it should be to the icon (or the whole row), which should fix the text automatically.

I was talking to Rusty this morning and he mentioned he'd prefer if they were aligned, so would be good if that can be fixed before we merge :-)

@patrickelectric
Copy link
Member Author

@joaoantoniocardoso check the latest version

Two categories: settings and tools

Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>
Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>
Two categories: settings and tools

Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>
Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>
core/frontend/src/menus.ts Outdated Show resolved Hide resolved
Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>
Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>
@patrickelectric patrickelectric merged commit 0843606 into bluerobotics:master Feb 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants