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

[GUI] Fix: Address Book menu hover background is white and text is white #907

Conversation

Rock-N-Troll
Copy link

@Rock-N-Troll Rock-N-Troll commented Mar 26, 2021

image

Current (Screenshot 1 -> Screenshot 2): hovering a selection shows a blank selection due to the background and the text turning white

New (Screenshot 1 -> Screenshot 3): hovering a selection shows a grey background correctly.
Aligns with grey #bababab used throughout the project.

@Rock-N-Troll Rock-N-Troll changed the title because hover is white and text is white, hover text is blank [GUI] Fix: Because hover background is white and text is white, display is blank Mar 26, 2021
@Rock-N-Troll Rock-N-Troll changed the title [GUI] Fix: Because hover background is white and text is white, display is blank [GUI] Fix: Address Book menu hover background is white and text is white Mar 26, 2021
@codeofalltrades codeofalltrades added Component: GUI Primarily related to the display of the user interface Tag: Waiting For Code Review Waiting for code review from a core developer labels Mar 27, 2021
Copy link
Collaborator

@codeofalltrades codeofalltrades left a comment

Choose a reason for hiding this comment

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

ACK 676242c
Win10

@WetOne
Copy link
Collaborator

WetOne commented Mar 28, 2021

All the other menus change the text color rather than change the background color. For consistency this should change the foreground color of the text on hover. I admit that (depending on your screen) that the other menus can be difficult to read (the thin font grey with white background). We should change this to black text for the non-hovered text (as current) and use a more bold color (blue) for the hovered text. This change should be rolled out over time to the other menus.

@CaveSpectre11, @codeofalltrades - thoughts?

@CaveSpectre11
Copy link
Collaborator

CaveSpectre11 commented Mar 28, 2021

We should change this to black text for the non-hovered text (as current) and use a more bold color (blue) for the hovered text. This change should be rolled out over time to the other menus.

I agree that it should be consistent, and I also agree that the light gray to black that exists on the My Addresses page isn't necessarily the right approach. I also notice that the select color in My Addresses and in the Transactions list is different then the color scheme of the Send page address book.

I think we should work towards bringing them all together, this PR can be used as a starting point and then other PRs for the other portions. There's two possible schemes that I see that would use existing layout elements and work for the mouseover; and that being the scheme the Transactions page, or the My Addresses page works. Either:

  1. the mouseover feel on those pages; black on white becomes dark gray on light gray
  2. The selection feel on those pages; dark gray on light blue.

I'm not sure which one would look better, but I'm leaning towards the second option

(1: second line, 2: third line)
Screenshot from 2021-03-28 15-23-51

@WetOne
Copy link
Collaborator

WetOne commented Mar 28, 2021

Of the two you mention, dark grey on light blue is better. Let me suggest a third: black text on light blue. The contrast would help the text stand out more.

@CaveSpectre11
Copy link
Collaborator

Of the two you mention, dark grey on light blue is better. Let me suggest a third: black text on light blue. The contrast would help the text stand out more.

I'm good with that; of course the existing My addresses and and Transaction page scheme would need to change slightly (to have the black text on light blue instead of gray on light blue) to maintain consistency.

@CaveSpectre11 CaveSpectre11 added Tag: Waiting For Developer and removed Tag: Waiting For Code Review Waiting for code review from a core developer labels Mar 30, 2021
Copy link
Collaborator

@CaveSpectre11 CaveSpectre11 left a comment

Choose a reason for hiding this comment

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

Technically sound; just a color scheme change for this (and can be kept with this even though there will be more places that will need this color scheme change).

Have the foreground stay black and have the background change to the light blue that is shown when you select an address on transaction on their relevant pages.

@WetOne
Copy link
Collaborator

WetOne commented Apr 1, 2021

I was thinking about this. Qt allows for a styleSheet for these menus. I haven't looked but would be willing to bet that the "My Addresses" and "Transactions" pages are using the same style sheet. If we change just the style sheet, one change would change all the screens that need to be changed - 1 file and maintain consistency. Worst case, we need to create a stylesheet for these and then have these screens use the stylesheet.

@Rock-N-Troll
Copy link
Author

Rock-N-Troll commented Apr 4, 2021

Moved the configuration to main.css in a css block for all
QMenu:item:selected

items along with black text

image

The blue I set (#6f9bf5) corresponds (closer but not exactly with it looks like) with the color used elsewhere in the wallet (like hovering to find # of confirmations)

image

The hover in the addressbook is a lighter blue than this, but the value is easily changed now that the css has been established as was agreed upon in the issue comment thread above.

@CaveSpectre11 CaveSpectre11 dismissed their stale review April 4, 2021 16:29

To re-review with changes

@CaveSpectre11 CaveSpectre11 added Tag: Waiting For Code Review Waiting for code review from a core developer and removed Tag: Waiting For Developer labels Apr 4, 2021
@CaveSpectre11
Copy link
Collaborator

remember to rebase/squash your commits to a single atomic commit for a single change,

move to css. Use darker blue than requested
@Rock-N-Troll Rock-N-Troll force-pushed the dropdown_menu_text_background branch from c405a0f to 1dba6c4 Compare April 5, 2021 02:49
Copy link
Collaborator

@CaveSpectre11 CaveSpectre11 left a comment

Choose a reason for hiding this comment

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

ACK 1dba6c4

@codeofalltrades
Copy link
Collaborator

ack 1dba6c4

@codeofalltrades codeofalltrades merged commit 83db278 into Veil-Project:master Apr 7, 2021
@Rock-N-Troll Rock-N-Troll deleted the dropdown_menu_text_background branch June 25, 2022 00:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: GUI Primarily related to the display of the user interface Tag: Waiting For Code Review Waiting for code review from a core developer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants