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

Make tooltips more visually pleasing #6097

Merged
merged 5 commits into from
Mar 30, 2023
Merged

Conversation

Procrat
Copy link
Contributor

@Procrat Procrat commented Mar 28, 2023

Pull Request Description

Closes #6059: visually centering text and adding a longer delay before showing tooltips. See the commit messages for details.

Recording 2023-03-28 at 09 43 17

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.

Two issues seem to be occurring simultaneously:
- The Text component updates its width and height separately. The Label
  component, which is part of tooltips, wasn't listening to updates to
  the height. At the start, it's still set to the wrong value.
- The baseline or height calculation of the M PLUS P1 font seems off.
  This difference is smaller. A separate issue was created for that:
  #6086
500ms seems to be the most frequently used delay before showing a
tooltip. Since the fade-in/fade-out animation already takes some time,
I've set the hide delay to 0.
@Procrat Procrat force-pushed the wip/procrat/nicer-tooltips-6059 branch from b3388b2 to 6bf4b7a Compare March 28, 2023 07:45
@Procrat Procrat marked this pull request as ready for review March 28, 2023 07:45
@wdanilo
Copy link
Member

wdanilo commented Mar 29, 2023

I can't find it easily in the code - what was the cause of the text not being centered? What was the main change in the code allowing for it to be displayed nicely now? :)

@Procrat
Copy link
Contributor Author

Procrat commented Mar 29, 2023

I can't find it easily in the code - what was the cause of the text not being centered? What was the main change in the code allowing for it to be displayed nicely now? :)

As I mentioned in the commit message, the Text component updates its width and height separately. The Label component, which is part of tooltips, wasn't listening to updates to the height, only to the width. So a change to the width would fire and the height would be set at that time, but an update to the height would happen afterwards which wasn't taken into account.

@wdanilo
Copy link
Member

wdanilo commented Mar 29, 2023

Gotcha! It's hard to read commit messages, as there might be a lot of commits, so no one really does it here. Please include such info in the future in the PR description :)

lib/rust/ensogl/component/label/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@vitvakatu vitvakatu left a comment

Choose a reason for hiding this comment

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

QA: passed

@Procrat Procrat added the CI: Ready to merge This PR is eligible for automatic merge label Mar 30, 2023
@mergify mergify bot merged commit ef45b6e into develop Mar 30, 2023
@mergify mergify bot deleted the wip/procrat/nicer-tooltips-6059 branch March 30, 2023 08:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make tooltips more visually pleasing
3 participants