Conversation
2842318 to
fd2571d
Compare
fd2571d to
41bb4ce
Compare
|
This looks really good. The only suggestions I have would be to make the icon red and the tooltip state that the synchronization process could take awhile if the user is synchronizing for the first time. Thoughts? |
|
@essofluffy: IMO red is the color of errors. Yellow would be appropriate. But since i started with black, flat icons (#5219) i would say black is fine and it would avoid rainbow color software. |
b8a068b to
81649a2
Compare
|
@jonasschnelli That PR had some good discussion so I'll concede on the color of the icon. Black will work just fine. |
|
Added another commit:
|
|
utAck, I like tooltips for verbiage, but the icon looks really blended in with the text, I'm not sure a user would notice it has additional information if hovered over. |
|
@jonasschnelli Did you make these icons? |
@essofluffy: no, check: https://github.com/jonasschnelli/bitcoin/blob/2015/05/qt_polish_1/doc/assets-attribution.md#typiconsstephen-hutchings
I have to agree. Placing the text "(out of sync)" next to the icon would be a option. Other ideas? |
Typically the visual cue i look for in a tooltip is a little |
|
Maybe something like, (out of sync, hover for more info)? |
|
Concept ACK |
|
@essofluffy @faizkhan00 : i don't think we should tell the user "hover for more information". It's somehow uncommon. |
|
@jonasschnelli Hmm, yeah looking over the images I think this is reasonable too, I'm correct in assuming those icons disappear once the blockchain is fully synced up, as well? |
|
@faizkhan00 your assumption would be correct. |
|
@fanquake: would you agree with 3) if the icon would be clickable then showing a info-alert with the extended out-of-sync information? What about 4)? (#6110 (comment)) |
|
ACK |
|
@jonasschnelli ACK 4 as well. If consensus is that 3 is an improvement, then lets get this merged. |
|
Concept ACK Significant issue: point 2 results in less recipients visible in sendcoins dialog. Nits/preferred changes:
|
|
@luke-jr Icon color has been already discussed above, IMO it's fine like this. It is not a very dangerous warning, we don't want to unnecessarily light up the overview page like a christmas tree. |
The new sendcoinsentries are some pixels higher. But it depends on you window height. So i think this is not very urgent.
Agreed. But this would be a different PR
As @laanwj said. See above. IMO red makes only sense for errors. |
|
@jonasschnelli I don't get the point of making sendcoinsentry taller. It doesn't depend on window height - no matter what height is used, less now fits in it. |
|
@luke-jr: Okay. The top/bottom margin could be better. I try to solve it and will see if i get ~the same height. |
81649a2 to
66ee4b6
Compare
# Conflicts: # src/qt/forms/overviewpage.ui # src/qt/overviewpage.cpp
66ee4b6 to
093ed95
Compare
|
@luke-jr: I did optimize the sendcoinsentry height. Check the comparison screenshot (open the image url in a new tab/window to get full resolution). I hope this PR makes it into 0.11! |
|
Tested ACK |
There was a problem hiding this comment.
I wonder if this will cause code-unreachable warnings, may be better to #else #ifdef the rest of the function, same for SingleColorIcon.
There was a problem hiding this comment.
Agreed. It's kinda stupid to try to compile code after return.
Fixed.
093ed95 to
ca5f688
Compare
ca5f688 [QT] don't colorize icons on win and mac (Jonas Schnelli) 7247d10 [QT] use alert icon with tooltip insted of "(out of sync)" text (Jonas Schnelli) 51c7c70 [QT] remove frame to avoid double-frame situation in sendcoinsentry.ui (Jonas Schnelli) 2a6b844 [QT] change transaction amount and height in overview page (Jonas Schnelli)
|
Posthumous nit: the warning icon doesn't get the color of the other single-color icons, it is black even on Linux |
|
Oh. Right. Will fix this. |
|
@laanwj I wouldn't expect it to? Looks like it really ought to be a considered part of the text now. :) |
|
@luke-jr Fine with me. But then it should color with the text, currently it is always black. |
|
Oh, I forgot sometimes text isn't black :) I wonder if we should just make a font for our single-colour icons at this point? (assuming Qt lets you use Unicode characters for icons... which it might not) |
|
Using a font for the icons would be a neat and modern solution. Could put the icons in the Private Use Area, and use them accordingly in (rich)text. I also proposed it in the beginning when the single-color-icons were introduced. Technically it could be somewhat more of a challenge, though - I don't know Qt-Custom-Font-Handling specifics. No desperate need though, that's the realm of 'far future would be nice'. E.g. no more custom code would be needed to color the icons. |
|
Most of the "new" icons are available as font (typicon font). But handling a font on three platform is probably much more complex the handling PNG files. |



I cumulate changes to avoid having to many PRs:
EDIT: added point 4
Screens from current master
Double frame issue:

Empty space:
