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

[Qt]: Improve sendcoinsdialog readability #13158

Merged
merged 1 commit into from May 14, 2018

Conversation

Projects
None yet
5 participants
@marcoagner
Copy link
Contributor

marcoagner commented May 3, 2018

I'm addressing two (probably duplicate) issues: #11606 and #10613.

Some points worth noting:

  • I've tried to balance the proposed changes on both issues without going too far and remaining a bit conservative. It will be easier to improve based on suggestions where necessary.

  • I preferred to maintain a layout that doesn't ask for an address truncation because, in my view, this wallet should be conservative on this.

  • I didn't follow the idea of aligning the amounts to the right for finding it more natural (and minimalist) to read the information without having to map alignments. Additionally, that approach seems to need more <hr />'s (or similar) in order to help the user to map information, which ended up cluttering the box too much (specially with multiple recipients). Thus, I preferred to just give some more space between recipients. Let me know if there are better ideas on this.

Visually, I went from this (current):
screenshot from 2018-05-03 15-11-30

To this:
screenshot from 2018-05-03 15-15-41

As a side note, while doing this, I thought about a better way to show fees and found there's already a PR on this (#12189) and thought it is

@fanquake fanquake added the GUI label May 3, 2018

@marcoagner marcoagner changed the title [qt]: Improve sendcoinsdialog readability [Qt]: Improve sendcoinsdialog readability May 3, 2018

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented May 3, 2018

Thanks for working on this. I haven't looked at the code, but the screenshots look good.

src/qt/sendcoinsdialog.cpp Outdated
@@ -337,19 +349,10 @@ void SendCoinsDialog::on_sendButton_clicked()
if(u != model->getOptionsModel()->getDisplayUnit())
alternativeUnits.append(BitcoinUnits::formatHtmlWithUnit(u, totalAmount));
}
questionString.append(tr("Total Amount %1")
questionString.append(tr("<b>Total Amount</b>: <b>%1</b>")

This comment has been minimized.

@marcoagner

marcoagner May 3, 2018

Author Contributor

This line may seem weird. But I made this line's bold this way to match the bold on "Transaction fee" line where the colon is not inside bold tag since I didn't think it was good to bold the entire "Transaction fee" line and there is more text between "Transaction fee" and the colon. Maybe someone has a better alternative.

This comment has been minimized.

@laanwj

laanwj May 7, 2018

Member

We should try, if possible, to keep the html/structure out of the translation strings, to make it easier for translators.
For example here it would be better to do this:

questionString.append("<b>%1</b>: <b>%2</b>".arg("Total Amount")
    .arg(...))

(this arises in a few other places as well)

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented May 7, 2018

Concept ACK

@jonasschnelli
Copy link
Member

jonasschnelli left a comment

Concept ACK
Build is here:
https://bitcoin.jonasschnelli.ch/build/597

src/qt/sendcoinsdialog.cpp Outdated
}

formatted.append(recipientElement);
}

QString questionString = tr("Are you sure you want to send?");
questionString.append("<br /><br />%1");
questionString.append(tr("<br /><span style='font-size:10pt;'>Please, review your transaction.</span><br />%1"));

This comment has been minimized.

@jonasschnelli

jonasschnelli May 7, 2018

Member

Please extract the translation part so translator do not have to deal with html tags, etc.

src/qt/sendcoinsdialog.cpp Outdated
questionString.append(BitcoinUnits::formatHtmlWithUnit(model->getOptionsModel()->getDisplayUnit(), txFee));
questionString.append("</span> ");
questionString.append(tr("added as transaction fee"));
questionString.append(tr("<hr /><b>Transaction fee</b>"));

This comment has been minimized.

@jonasschnelli

jonasschnelli May 7, 2018

Member

same here

src/qt/sendcoinsdialog.cpp Outdated
questionString.append("</span>");

questionString.append(QString("<br /><span style='font-size:10pt; font-weight:normal;'>(=%1)</span>")
.arg(alternativeUnits.join(" " + tr("or "))));

This comment has been minimized.

@jonasschnelli

jonasschnelli May 7, 2018

Member

use tr("or")+" "?

@marcoagner marcoagner force-pushed the marcoagner:improve_sendcoinsdialog_readability branch May 7, 2018

@marcoagner

This comment has been minimized.

Copy link
Contributor Author

marcoagner commented May 7, 2018

Thank you all for taking the time to review this.
As pointed, I extracted the html tags from translator.
The branch is rebased with master; should I squash the commits?
Let me know if you see something else I should address, thanks.

@marcoagner marcoagner force-pushed the marcoagner:improve_sendcoinsdialog_readability branch May 7, 2018

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented May 8, 2018

utACK 617dfa98ce16d461009cb5e9eb207c0e0eae30fe Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits

[qt]: changes sendcoinsdialog's box layout for improved readability.
[qt]: extracts html tags from translator.

[qt]: removes missed tr() call.

@marcoagner marcoagner force-pushed the marcoagner:improve_sendcoinsdialog_readability branch to f08a385 May 8, 2018

@marcoagner

This comment has been minimized.

Copy link
Contributor Author

marcoagner commented May 8, 2018

Done; commits squashed.

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented May 8, 2018

utACK f08a385

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented May 10, 2018

@jonasschnelli Mind to do a build here?

@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented May 11, 2018

@MarcoFalke MarcoFalke merged commit f08a385 into bitcoin:master May 14, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

MarcoFalke added a commit that referenced this pull request May 14, 2018

Merge #13158: [Qt]: Improve sendcoinsdialog readability
f08a385 [qt]: changes sendcoinsdialog's box layout for improved readability. (marcoagner)

Pull request description:

  I'm addressing two (probably duplicate) issues: #11606 and #10613.

  Some points worth noting:

  - I've tried to balance the proposed changes on both issues without going too far and remaining a bit conservative. It will be easier to improve based on suggestions where necessary.

  - I preferred to maintain a layout that doesn't ask for an address truncation because, in my view, this wallet should be conservative on this.

  - I didn't follow the idea of aligning the amounts to the right for finding it more natural (and minimalist) to read the information without having to map alignments. Additionally, that approach seems to need more `<hr />`'s (or similar) in order to help the user to map information, which ended up cluttering the box too much (specially with multiple recipients). Thus, I preferred to just give some more space between recipients. Let me know if there are better ideas on this.

  Visually, I went from this (current):
  ![screenshot from 2018-05-03 15-11-30](https://user-images.githubusercontent.com/5016303/39581859-16abec82-4edc-11e8-86d3-eb722f8a7ed6.png)

  To this:
  ![screenshot from 2018-05-03 15-15-41](https://user-images.githubusercontent.com/5016303/39582066-96856adc-4edc-11e8-804c-468aec44cc8d.png)

  As a side note, while doing this, I thought about a better way to show fees and found there's already a PR on this (#12189) and thought it is

Tree-SHA512: e94b740fab6c1babd853a97be65c3b6f86ec174c975a926fde66b147f7a47e0cf0fa10f7255ba92aaba68c76a80dde8c688008179a34705a9799bf24d3c5cd46
@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented May 16, 2018

Post merge tested ACK f08a385

@marcoagner marcoagner deleted the marcoagner:improve_sendcoinsdialog_readability branch Jul 19, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.