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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

gui: Improved tooltip for send amount field #17180

Merged
merged 1 commit into from Oct 23, 2019

Conversation

@JeremyCrookshank
Copy link
Contributor

JeremyCrookshank commented Oct 17, 2019

I noticed that on Bitcoin sends the tooltip wasn't very clear for new users and I hope my PR is more concise. If it needs changing more will happily change too 馃憤
IMG_20191017_192739

@fanquake fanquake added the GUI label Oct 17, 2019
@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Oct 17, 2019

Is this a leftover from BIP 70?

ACK

@dannmat

This comment has been minimized.

Copy link

dannmat commented Oct 17, 2019

ACK

@JeremyCrookshank

This comment has been minimized.

Copy link
Contributor Author

JeremyCrookshank commented Oct 17, 2019

@MarcoFalke MarcoFalke added this to the 0.20.0 milestone Oct 17, 2019
Copy link
Contributor

kristapsk left a comment

ACK 31cf331

@MarcoFalke

This comment has been minimized.

@MarcoFalke MarcoFalke removed this from the 0.20.0 milestone Oct 18, 2019
@fanquake fanquake changed the title gui: Improved wording/explanation of Bitcoin sends amount box gui: Improved tooltip for send amount field Oct 18, 2019
Copy link
Member

fanquake left a comment

ACK 31cf331

master (fd3b4e4):
master

PR (31cf331):
pr

@GChuf

This comment has been minimized.

Copy link
Contributor

GChuf commented Oct 20, 2019

ACK 31cf331

@promag

This comment has been minimized.

Copy link
Member

promag commented Oct 20, 2019

How about "the amount to send" or "the amount to send in the selected unit"?

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Oct 21, 2019

Agree with promag. I don't think you even need to mention bitcoin. I'd prefer "the amount to send in the selected unit".

@JeremyCrookshank

This comment has been minimized.

Copy link
Contributor Author

JeremyCrookshank commented Oct 21, 2019

How about if I have the tooltip say the unit selected?

IE
The amount of Satoshis to send
The amount of mBTC to send

@promag

This comment has been minimized.

Copy link
Member

promag commented Oct 21, 2019

Probably nice but IMO not worth the effort. A constant tooltip is fine.

@GChuf

This comment has been minimized.

Copy link
Contributor

GChuf commented Oct 21, 2019

I agree with "amount to send" or "amount to send in selected unit"

@JeremyCrookshank JeremyCrookshank force-pushed the JeremyCrookshank:sendamounttooltip branch from 001bb8b to 60d226e Oct 21, 2019
@JeremyCrookshank JeremyCrookshank deleted the JeremyCrookshank:sendamounttooltip branch Oct 21, 2019
@JeremyCrookshank

This comment has been minimized.

Copy link
Contributor Author

JeremyCrookshank commented Oct 21, 2019

Had some issues with Git(still need to squash them?). I have however committed my PR which means the tooltip now reflects the selected unit appropriately. I will post photos tomorrow unless someone beats me to it.

src/qt/forms/sendcoinsentry.ui Outdated Show resolved Hide resolved
src/qt/bitcoinamountfield.cpp Outdated Show resolved Hide resolved
@JeremyCrookshank JeremyCrookshank force-pushed the JeremyCrookshank:sendamounttooltip branch from 1c3b9bb to ddc3cf2 Oct 22, 2019
@JeremyCrookshank

This comment has been minimized.

Copy link
Contributor Author

JeremyCrookshank commented Oct 22, 2019

I have made the changes as requested

@@ -18,7 +18,7 @@
</property>
<widget class="QFrame" name="SendCoins">
<property name="toolTip">
<string>This is a normal payment.</string>
<string>The amount to send in the selected unit</string>

This comment has been minimized.

Copy link
@promag

promag Oct 23, 2019

Member

Could be the payAmount tooltip, in line 168?

This comment has been minimized.

Copy link
@laanwj

laanwj Oct 23, 2019

Member

Let's not keep drawing this out. The previous tooltip was wrong and this one is correct.

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Oct 23, 2019

ACK 088a730

laanwj added a commit that referenced this pull request Oct 23, 2019
088a730 static tooltip (JeremyCrookshank)

Pull request description:

  I noticed that on Bitcoin sends the tooltip wasn't very clear for new users and I hope my PR is more concise. If it needs changing more will happily change too 馃憤
  ![IMG_20191017_192739](https://user-images.githubusercontent.com/46864828/67036925-75d45380-f114-11e9-88bf-bab58161f80a.jpg)

ACKs for top commit:
  laanwj:
    ACK 088a730

Tree-SHA512: 2b1103ac934d8f68d22333af3c0f5d4228b665b1e507378d4ae5b83cc2b6d6aeb46a3d68298cca93feb839db5caa560322c8df5261dc2f7db5abeed9f0dd9c69
@laanwj laanwj merged commit 088a730 into bitcoin:master Oct 23, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Nov 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants
You can鈥檛 perform that action at this time.