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

Introducing new high contrast themes #2522

Merged
merged 4 commits into from
Feb 24, 2023
Merged

Introducing new high contrast themes #2522

merged 4 commits into from
Feb 24, 2023

Conversation

rdwebdesign
Copy link
Member

@rdwebdesign rdwebdesign commented Feb 8, 2023

What does this PR aim to accomplish?

This PR adds 2 more themes to the web interface.

The new themes are an attempt to improve the visualization of some important items (allow/blocked queries, network table items, etc) even without colors.

screenshot

I tried to keep different "states" discernible, even without the idea "Green=OK/Success" and "Red=Bad/Error".
This is very common for normal color view, but this is far from "accessible" when we talk about low-contrast or color-blindness.

How does this PR accomplish the above?

  • Using better contrast ratio between text and backgrounds for most UI elements;
  • Using color palettes designed to keep the element discernible, even when some or all colors are not present.
  • Changing the colors used to highlight allowed/blocked queries.
  • Changing the colors used to differentiate items on the network table.
  • Using a white logo, because the red/green logo looks very ugly for most color-blind users.

Link documentation PRs if any are needed to support this PR:

none.


By submitting this pull request, I confirm the following:

  1. I have read and understood the contributors guide, as well as this entire template. I understand which branch to base my commits and Pull Requests against.
  2. I have commented my proposed changes within the code and I have tested my changes.
  3. I am willing to help maintain this change if there are issues with it later.
  4. It is compatible with the EUPL 1.2 license
  5. I have squashed any insignificant commits. (git rebase)
  6. I have checked that another pull request for this purpose does not exist.
  7. I have considered, and confirmed that this submission will be valuable to others.
  8. I accept that this submission may not be used, and the pull request closed at the will of the maintainer.
  9. I give this submission freely, and claim no ownership to its content.

  • I have read the above and my PR is ready for review. Check this box to confirm

@rdwebdesign rdwebdesign force-pushed the high_contrast branch 2 times, most recently from 80a714e to b92ce0e Compare February 8, 2023 21:39
@PromoFaux
Copy link
Member

Please provide some sample screenshots in the OP

@rdwebdesign
Copy link
Member Author

Rebased.

- create a new function to access CSS values, even if not directly used on the page;
- remove the empty harcoded HTML `<span>` placeholders used only to source the colors from CSS classes

Signed-off-by: RD WebDesign <github@rdwebdesign.com.br>
Signed-off-by: RD WebDesign <github@rdwebdesign.com.br>
Copy link
Member

@yubiuser yubiuser left a comment

Choose a reason for hiding this comment

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

Do we need btn-blacklist and btn-whitelist class in the query log as well?

@rdwebdesign
Copy link
Member Author

I don't think they are needed, but we can include the classes later if with find a reason to do it.

I added this classes on the auditlog because there were no previous selector to apply a CSS to them and this buttons are different on this page.
They are smaller and they had a very poor contrast on the light theme.

- change HTML header code
- add new class to Pi-hole logo
- add new classes to auditlog buttons
- add new classes to query log and long-term query log buttons

Signed-off-by: RD WebDesign <github@rdwebdesign.com.br>
@rdwebdesign rdwebdesign force-pushed the high_contrast branch 2 times, most recently from 9fcef6f to 970c2b8 Compare February 16, 2023 04:29
@rdwebdesign
Copy link
Member Author

After playing with both themes, I made a few more changes to improve the visibility of some buttons. I added the classes. Now the buttons on query log will look exactly like the buttons on audit log page.

@rdwebdesign rdwebdesign force-pushed the high_contrast branch 2 times, most recently from 2dc1c62 to de5964a Compare February 18, 2023 04:36
yubiuser
yubiuser previously approved these changes Feb 19, 2023
Signed-off-by: RD WebDesign <github@rdwebdesign.com.br>
@rdwebdesign
Copy link
Member Author

I removed one unnecessary CSS rule.

@rdwebdesign rdwebdesign merged commit 44f3b41 into devel Feb 24, 2023
@rdwebdesign rdwebdesign deleted the high_contrast branch February 24, 2023 00:25
@pralor-bot
Copy link

This pull request has been mentioned on Pi-hole Userspace. There might be relevant details there:

https://discourse.pi-hole.net/t/pi-hole-ftl-v5-22-web-v5-19-and-core-v5-16-1-released/61999/1

@saint-lascivious
Copy link

saint-lascivious commented Mar 23, 2023

I can't replicate it on my end without pissing about changing the client colour selection logic (or perhaps creating a bunch of bogus clients - I run Pi-hole behind load balancers and only see the proxies as clients), but I noticed in the example image provided that the first spike of client traffic into the 400s is almost completely invisible in the dark theme in either low brightness or high glare scenarios.

Apologies getting to this rather late in the game, I was just now reviewing changes after the release announcement.

If it's of any additional use or relevance (now or future) I'm almost entirely red blind, but a normally sighted housemate also confirms the client traffic contrast issue with the dark theme as described above.

Edited to add: This appears to be a difficult problem to fix with very large numbers of clients. Perhaps entirely unrelated to this theme specifically.

Ensuring the selected colour is meaningfully distinct and has a sufficient foreground/background contrast ratio becomes more and more difficult as the number of clients increase.

@rdwebdesign
Copy link
Member Author

This PR was intended to adapt the user interface (menus, buttons, table rows, etc), but the graphics (bar and doughnut) were not touched here.

I still want to improve these graphics, but they are really challenging, as you noticed.
Sometimes the clients graphic uses too many colors and is very difficult to find a good colorblind palette.
Also, there are more than one type of colorblindness, making the job even more difficult.

@saint-lascivious
Copy link

saint-lascivious commented Mar 23, 2023

Sometimes the clients graphic uses too many colors and is very difficult to find a good colorblind palette.

Spitballing here and I hope that's ok.

I was thinking some more on this, and it seems the thing that makes this truly challenging is that the highly variable number of clients in any given deployment.

If switching client colour pallets based on the number of clients isn't out of the question (I believe the current logic is deterministic?), then it seems it might be possible to solve this in (perhaps major) sub-set of cases with a conditional approach.

For example, use pallet X when there's 8 clients or less, use Y when there's 9 through 16 clients, Z for 17 through 32, and the current logic past here where it gets extremely difficult to offer guaranteed accessible colours.

Another approach entirely might be to just display each bar in a single solid colour, and display a client donut graph on click or mouseover with individual client contributions from that graph period. Perhaps combined with the above mentioned conditional pallets.

It's quite possible and indeed probable that I'm overthinking this.

Edit: correct formatting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants