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

New ... menu in the top bar with zoom controls #9073

Conversation

MichaelMauderer
Copy link
Contributor

@MichaelMauderer MichaelMauderer commented Feb 15, 2024

Pull Request Description

Closes #8614

0001-0093.mp4

Important Notes

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • The documentation has been updated, if necessary.
  • Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
  • All code follows the
    Scala,
    Java,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.
  • All code has been tested:
    • Unit tests have been written where possible.
    • If GUI codebase was changed, the GUI was tested when built using ./run ide build.

@MichaelMauderer MichaelMauderer self-assigned this Feb 15, 2024
@MichaelMauderer MichaelMauderer added CI: No changelog needed Do not require a changelog entry for this PR. -gui labels Feb 15, 2024
@MichaelMauderer MichaelMauderer marked this pull request as ready for review February 15, 2024 13:45
@somebody1234
Copy link
Contributor

I think it's missing backdrop-filter: blur(64px):
image

Also not sure about the very large space between the word "zoom" and the controls.
My personal thoughts on the design:

  • "Zoom" label is not needed
  • the divider is not needed - or at least, it shouldn't be that noticeable
  • I'd personally prefer if the buttons had no outline
  • and transition: background-color 0.3s plus background-color: rgba(255 255 255 / 0.3) or the like on hover
  • cursor: pointer on the buttons
  • Optional: title attribute on buttons

@MichaelMauderer
Copy link
Contributor Author

I think it's missing backdrop-filter: blur(64px): ![image](https://private-user-images.githubusercontent.com/

Fixed. The filter was there, but it was not applied due to some nesting of elements.

Also not sure about the very large space between the word "zoom" and the controls. My personal thoughts on the design:

* "Zoom" label is not needed

* the divider is not needed - or at least, it shouldn't be that noticeable

* I'd personally prefer if the buttons had no outline

* and `transition: background-color 0.3s` plus `background-color: rgba(255 255 255 / 0.3)` or the like on hover

* `cursor: pointer` on the buttons

* Optional: `title` attribute on buttons

I updated the design. I left the label and the divider as they have been part of the proposed design (and are used in existing similar implementations. The label does help to make it clear what the controls are at a glance.

Peek.2024-02-16.14-42.webm

@somebody1234
Copy link
Contributor

somebody1234 commented Feb 16, 2024

looks quite a bit cleaner now. still not too sure about a few things though:

  • the gap is still there. it's true that it's there on the original design, but i'm not sure it's a good idea to have a random gap for no apparent reason
  • a few of the controls are slightly above center (the divider and the fullscreen button, possibly also the "Zoom" text).
  • the + icon looks a bit asymmetric (the left side of the - looks a bit shorter). not sure whether that's an issue with this specific zoom... but still something that should probably be fixed given that this is the default UI zoom level

@farmaazon
Copy link
Contributor

  • the gap is still there. it's true that it's there on the original design, but i'm not sure it's a good idea to have a random gap for no apparent reason

I think we expect to have more positions in this menu soon, and then both the gap and label would make sense. If we don't make any such entries in a long time, then it will be easy to remove both the label and the gap.

Copy link
Contributor

@farmaazon farmaazon left a comment

Choose a reason for hiding this comment

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

I don't understand why do we need so many before and after elements, and what role they play.

@somebody1234
Copy link
Contributor

it looks like the icons are implemented in pure CSS using pseudo-elements. i think they should probably be added to the icon spritesheet instead

@MichaelMauderer MichaelMauderer added the CI: Ready to merge This PR is eligible for automatic merge label Feb 19, 2024
Copy link
Contributor

@farmaazon farmaazon left a comment

Choose a reason for hiding this comment

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

I'm ok with merging it for now. But I think it would be better to use icons from our big svg - but that require a bit of figma work.

@mergify mergify bot merged commit e264189 into develop Feb 20, 2024
26 of 29 checks passed
@mergify mergify bot deleted the wip/MichaelMauderer/New-extended-menu-in-the-top-bar-with-zoom-controls branch February 20, 2024 15:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-gui CI: No changelog needed Do not require a changelog entry for this PR. CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New ... menu in the top bar with zoom controls
3 participants