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

Peertableview alternating row colors #298

Merged
merged 1 commit into from
Apr 30, 2021
Merged

Peertableview alternating row colors #298

merged 1 commit into from
Apr 30, 2021

Conversation

RandyMcMillan
Copy link
Contributor

@RandyMcMillan RandyMcMillan commented Apr 25, 2021

peers-tab: enable alternating row colors for peer table and banned table

Copy link
Member

@jarolrod jarolrod left a comment

Choose a reason for hiding this comment

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

Concept ACK

The other table views such as in the Receive and Transactions tab use alternating row colors. Seems appropriate that we also enable this on the Peers table.

@@ -925,6 +925,9 @@
</property>
<property name="textElideMode">
<enum>Qt::ElideMiddle</enum>
</property>
Copy link
Member

Choose a reason for hiding this comment

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

You're introducing this property within the textElideMode property.

You should be introducing this after line 928 on the original file, not after line 927. That way you don't have to introduce this closing tag to then introduce the property.

Also, the indentation is off here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just trying to edit .ui files without qt-creator - the diff junk qt-creator makes is annoying.

Copy link
Member

Choose a reason for hiding this comment

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

I'm using Qt Designer to edit UI files.

Yeah, in most cases diff junk is unavoidable, but it is easy for me to revert it in Sublime Merge :)

@RandyMcMillan
Copy link
Contributor Author

e94920a

Screen Shot 2021-04-26 at 7 31 35 PM (2)

Copy link
Member

@jarolrod jarolrod left a comment

Choose a reason for hiding this comment

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

tACK e94920a

Can you edit your PR description to include a sentence(s) explaining this change? Normally we don't leave a PR description empty or only fill it with images. Additionally, edit the PR title to remove the leading qt:. This is redundant as it's known this is in the GUI repo and the commit title already includes this.

Thanks for also adding this to the Banned Peers table
Screen Shot 2021-04-26 at 10 36 06 PM

@hebasto hebasto added the Design label Apr 28, 2021
@hebasto
Copy link
Member

hebasto commented Apr 28, 2021

@Bosch-0, @GBKS

Do you mind having a look into this PR as designers?

@jonatack
Copy link
Member

I really appreciate @RandyMcMillan's initiatives to improve the GUI and want to encourage them. In this case, it's maybe a case of "just because we can may not mean that we should": (a) separating info for a single peer not only isn't as critical as it might be for a transaction, but the peers detail area already exists for this, (b) the peers table columns can be as useful (or more useful) information as the the rows (in the peers table, I tend to read the columns) and adding horizontal lines would make that more difficult, and (c) it's more cluttered and less minimal a design without a real need AFAICT.

@RandyMcMillan
Copy link
Contributor Author

I think this subtle change is useful - alt row colors is a common attribute to
many UIs - and I would agree - striking the right balance is critical.

UI-Patterns - AlternatingRowColors

Screen Shot 2021-04-28 at 12 07 36 PM

Screen Shot 2021-04-28 at 12 12 50 PM

This gif offers many variations on table organization:

It is an easy change that helps organize textual information especially as the table gets larger.

@Bosch-0
Copy link

Bosch-0 commented Apr 28, 2021

tACK e94920a on Windows 10 - works as intended. Before / after below:

Frame 43

I would agree that this is a slight UI/UX improvement though I understand Jon's concerns. The peers tab isn't something I've used too often so I can not attest to the value of reading info horizontally or vertically, but horizontal alternating row colors is a pretty common design for tables that people will be familiar with and it makes things less visually cluttered.

@hebasto hebasto changed the title qt: peertableview alternating row colors Peertableview alternating row colors Apr 30, 2021
@hebasto hebasto merged commit 13f24d1 into bitcoin-core:master Apr 30, 2021
@jonatack
Copy link
Member

I think this change makes this window needlessly noisy, more cluttered, and more difficult to use. It was the main tab I used. Oh well.

@RandyMcMillan
Copy link
Contributor Author

@jonatack - Maybe we can add more gui options to the user preferences - This may be a great direction to go. I like @hebasto's addition of the monospace font option - I have plenty of ideas for accessibility that would be great to be able to toggle on/off.

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request May 1, 2021
e94920a qt: peertableview alternating row colors (randymcmillan)

Pull request description:

  peers-tab: enable alternating row colors for peer table and banned table

ACKs for top commit:
  Bosch-0:
    tACK bitcoin-core/gui@e94920a on Windows 10 - works as intended. Before / after below:
  jarolrod:
    tACK e94920a

Tree-SHA512: 05ba18e1db9700bbd68644fe02292409f4e5c52e301b1b2977c335d1ff16456a93fb0b15c8c8385d1b15f648141341990706d530f6b08ecb33098fa941b9af1f
@rebroad
Copy link
Contributor

rebroad commented May 1, 2021

Please can this change be reverted? I agree with @jonatack - there's way too much info in the right pane now, and I don't see the need for the alternating colours - I never had an issue following the lines - did anyone else?

Maybe re-merge this in the future once it's made possible to select which features people want?

@rebroad
Copy link
Contributor

rebroad commented May 1, 2021

tACK e94920a on Windows 10 - works as intended. Before / after below:

Frame 43

I would agree that this is a slight UI/UX improvement though I understand Jon's concerns. The peers tab isn't something I've used too often so I can not attest to the value of reading info horizontally or vertically, but horizontal alternating row colors is a pretty common design for tables that people will be familiar with and it makes things less visually cluttered.

Hold on a moment.. Those two screenshots aren't a comparison of before and after just this change, are they? Looks like many changes between those two examples...

@promag
Copy link
Contributor

promag commented May 2, 2021

ACK, this makes table presentation consistent.

PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Oct 20, 2021
e94920a qt: peertableview alternating row colors (randymcmillan)

Pull request description:

  peers-tab: enable alternating row colors for peer table and banned table

ACKs for top commit:
  Bosch-0:
    tACK bitcoin-core/gui@e94920a on Windows 10 - works as intended. Before / after below:
  jarolrod:
    tACK e94920a

Tree-SHA512: 05ba18e1db9700bbd68644fe02292409f4e5c52e301b1b2977c335d1ff16456a93fb0b15c8c8385d1b15f648141341990706d530f6b08ecb33098fa941b9af1f
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Oct 21, 2021
e94920a qt: peertableview alternating row colors (randymcmillan)

Pull request description:

  peers-tab: enable alternating row colors for peer table and banned table

ACKs for top commit:
  Bosch-0:
    tACK bitcoin-core/gui@e94920a on Windows 10 - works as intended. Before / after below:
  jarolrod:
    tACK e94920a

Tree-SHA512: 05ba18e1db9700bbd68644fe02292409f4e5c52e301b1b2977c335d1ff16456a93fb0b15c8c8385d1b15f648141341990706d530f6b08ecb33098fa941b9af1f
pravblockc pushed a commit to pravblockc/dash that referenced this pull request Nov 18, 2021
e94920a qt: peertableview alternating row colors (randymcmillan)

Pull request description:

  peers-tab: enable alternating row colors for peer table and banned table

ACKs for top commit:
  Bosch-0:
    tACK bitcoin-core/gui@e94920a on Windows 10 - works as intended. Before / after below:
  jarolrod:
    tACK e94920a

Tree-SHA512: 05ba18e1db9700bbd68644fe02292409f4e5c52e301b1b2977c335d1ff16456a93fb0b15c8c8385d1b15f648141341990706d530f6b08ecb33098fa941b9af1f
gades pushed a commit to cosanta/cosanta-core that referenced this pull request May 9, 2022
e94920a qt: peertableview alternating row colors (randymcmillan)

Pull request description:

  peers-tab: enable alternating row colors for peer table and banned table

ACKs for top commit:
  Bosch-0:
    tACK bitcoin-core/gui@e94920a on Windows 10 - works as intended. Before / after below:
  jarolrod:
    tACK e94920a

Tree-SHA512: 05ba18e1db9700bbd68644fe02292409f4e5c52e301b1b2977c335d1ff16456a93fb0b15c8c8385d1b15f648141341990706d530f6b08ecb33098fa941b9af1f
gwillen pushed a commit to ElementsProject/elements that referenced this pull request Jun 1, 2022
@bitcoin-core bitcoin-core locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants