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

Introduce Tooltip for the BlockClock navbar button #400

Merged
merged 1 commit into from
May 27, 2024

Conversation

johnny9
Copy link
Contributor

@johnny9 johnny9 commented May 1, 2024

The Tooltip will appear when hovering over the BlockClock tab button in the desktop navigation bar. It will show the current state of IBD.

Two things are different than the figma. The wording for remaining blocks matches what is shown on the BlockClock instead of what the figma shows and After sync, Blockheight is shown instead. Looking for opinions on both during review.

vlcsnap-2024-05-01-20h05m06s361

Build Artifacts

@johnny9
Copy link
Contributor Author

johnny9 commented May 2, 2024

Update from dc31a22 to a7e4faa

  • Now with tooltip

Copy link
Contributor

@MarnixCroes MarnixCroes left a comment

Choose a reason for hiding this comment

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

7453346
there should also be a tooltip state for when node is offline/stopped, i.e:
I pause the blockclock -> tooltip keeps displaying the blocktime from last online/sync time.
doesn't seem to be right

@johnny9
Copy link
Contributor Author

johnny9 commented May 3, 2024

7453346 there should also be a tooltip state for when node is offline/stopped, i.e: I pause the blockclock -> tooltip keeps displaying the blocktime from last online/sync time. doesn't seem to be right

Good point. I will add some basic state status for paused/connecting/connected. I think i will just go with what the block clock currently displays then have the designers review.

Copy link
Contributor

@pablomartin4btc pablomartin4btc left a comment

Choose a reason for hiding this comment

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

tACK 7453346

I agree with wording about time "left" matching the BlockClock instead of "to go" as in figma, perhaps figma needs to be updated @GBKS?
  • Figma:

    image

  • This PR:

    image

It would be good to add the figma link and screenshot comparison in the description so reviewers could ACK easier.

I also agree with @johnny9 and @MarnixCroes about adding different states wording as currently one can see the some inconsistencies:

Downloading blocks (this one looks correct)

image

Paused

Screenshot from 2024-05-06 11-53-38

Connecting (after pausing)

image

Connecting (after sync completed/ blocktime determined)

image

Blocktime determined (this one looks correct)

image

@johnny9
Copy link
Contributor Author

johnny9 commented May 7, 2024

Update from 7453346 to bd62c7c

  • Added paused and connecting stated

Screenshot from 2024-05-06 21-59-03
Screenshot from 2024-05-06 21-58-56
Screenshot from 2024-05-06 21-58-53

@johnny9
Copy link
Contributor Author

johnny9 commented May 9, 2024

Update from bd62c7c to 2478b9e

  • newlline at end of file
  • Added copyright header to Tooltip.qml

@GBKS
Copy link
Contributor

GBKS commented May 10, 2024

Good one. I tested and found a few things.

First, the screenshots:

image

As you can see, the text disappears in light mode, and the color of the tip does not adjust. On the web, it's possible to tie SVG colors to CSS color variables that automatically change when the theme changes. Not sure what the technique is for QML.

For me, it switched straight from "Downloading blocks - Estimating" to "Blocktime - xyz". It then counted blocks from ~1.5M or so to the current number. For much of that time it showed the almost full red circle, which felt like something was broken.

The tooltip should be placed 4-5px higher up, so the arrow tip overlaps with the button. From Figma:
image

Good call with the extra states. Can we list out which states we need? I started putting some of them together.
image

@GBKS
Copy link
Contributor

GBKS commented May 10, 2024

Also a question. I see text directly in the code. To simplify localization, should those be text reference IDs instead so the actual copy can be grabbed from a locale-specific dictionary?

@johnny9
Copy link
Contributor Author

johnny9 commented May 10, 2024

Also a question. I see text directly in the code. To simplify localization, should those be text reference IDs instead so the actual copy can be grabbed from a locale-specific dictionary?

the qsTr() wrapper function is how localization is managed with Qt

@johnny9 johnny9 force-pushed the tooltip branch 2 times, most recently from 1dbe3ee to c97e4e5 Compare May 11, 2024 03:44
The Tooltip will appear when hovering over the BlockClock tab
button in the desktop navigation bar. It will show the current
state of IBD.
@johnny9
Copy link
Contributor Author

johnny9 commented May 11, 2024

For me, it switched straight from "Downloading blocks - Estimating" to "Blocktime - xyz". It then counted blocks from ~1.5M or so to the current number. For much of that time it showed the almost full red circle, which felt like something was broken.

Unfortunately that is an old bug we're aware of. It has to do with how we determine if IBD is finished or not.

Good call with the extra states. Can we list out which states we need?

I think those are the main ones at the moment. "Could not connect" is not included in my commit as we don't currently have that state until we integrate Network detection. Additionally, we'll probably want some error status as well.

@johnny9
Copy link
Contributor Author

johnny9 commented May 11, 2024

Update to to 8f8341f:

  • Fix tooltip text in lightmode
  • Adjusted the anchor upward 5 pixels so it aligns with the main component of the BlockClock mini button
  • Added light mode version of the tooltip arrow

Copy link
Contributor

@D33r-Gee D33r-Gee left a comment

Choose a reason for hiding this comment

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

tACK 8f8341f on WSL Ubuntu 22.04 LGTM

Screenshot

Screenshot 2024-05-14 110447

Copy link
Contributor

@MarnixCroes MarnixCroes left a comment

Choose a reason for hiding this comment

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

tack 8f8341f

one q: why are tooltip-arrow-dark.png and tooltip-arrow-light.png different size?

Copy link
Contributor

@pablomartin4btc pablomartin4btc left a comment

Choose a reason for hiding this comment

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

re-tACK 8f8341f

Checked all changes from 2478b9e as well.

@hebasto hebasto merged commit a7ccfc3 into bitcoin-core:main May 27, 2024
8 of 9 checks passed
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.

6 participants