Skip to content

Conversation

@cowellbunga
Copy link

@cowellbunga cowellbunga commented Sep 17, 2024

Thank you in advance for helping us to improve Terminal for MkDocs
Please read through the template below and answer all relevant questions. Your additional work here is greatly appreciated and will help us respond as quickly as possible. To avoid duplicates, please search existing Issues before submitting one here.

By submitting a PR to this repository, you agree to the terms within the Terminal for MkDocs Code of Conduct. Please see the contributing guidelines for how to create and submit a high-quality PR for this repo.

Description

Describe the purpose of this PR along with any background information and the impacts of the proposed change. For the benefit of the community, please do not assume prior context.

Provide details that support your chosen implementation, including: breaking changes, alternatives considered, changes to the API, etc.

If the UI is being changed, please provide before/after screenshots.

This PR adds a palette review script to tests/utils that outputs a markdown report in tests/.

This PR also includes some new high contrast palettes to compare via the review script.

I also removed the comment lines from the gruvbox dark palette because it was breaking the process and it is the only palette like this.

Some minor changes were made to the test scripts. html.py has conflicts with the requests package, so it was renamed to html_utils.py. Also deleted the version line of the docker compose file because it was raising a warning.

References

Include any links supporting this change such as a:

  • GitHub Issue/PR number addressed or fixed
  • Related pull requests/issues from other repos

If there are no references, simply delete this section.

This PR addresses #120 and #119 in an attempt to expand on the palette options and to expedite the palette review process.

Testing

Describe how this can be tested by reviewers. Be specific about anything not tested and reasons why. If there are existing unit and/or integration test suites, tests should be added for new functionality and existing tests should complete without errors.

Please include any manual steps for testing end-to-end or functionality not covered by unit/integration tests.

Also include details of the environment this PR was developed in (language/platform/browser version).

  • This change adds test coverage for new/changed/fixed functionality
  • No tests added because the review script is almost a test itself, but some default (commonly used) palettes could be added to test the accuracy of the review.

Checklist

  • I have added documentation for new/changed functionality in this PR or in mkdocs-terminal/documentation
  • All active GitHub checks for tests, formatting, and security are passing

@ntno ntno self-requested a review September 30, 2024 22:23
@ntno ntno changed the base branch from main to rename-utils-file September 30, 2024 22:27
@ntno
Copy link
Owner

ntno commented Sep 30, 2024

closing PR to refresh the diff file, will reopen

@ntno ntno closed this Sep 30, 2024
@ntno ntno reopened this Sep 30, 2024
@ntno ntno changed the base branch from rename-utils-file to main September 30, 2024 22:29
@ntno
Copy link
Owner

ntno commented Sep 30, 2024

didn't work, will try again but update the base while PR is closed

@ntno ntno closed this Sep 30, 2024
@ntno
Copy link
Owner

ntno commented Sep 30, 2024

can't change the base branch while the PR is closed; reopening

@ntno ntno reopened this Sep 30, 2024
@ntno ntno changed the base branch from main to rename-utils-file September 30, 2024 22:31
@ntno ntno changed the base branch from rename-utils-file to main September 30, 2024 22:32
@ntno
Copy link
Owner

ntno commented Sep 30, 2024

hi @cowellbunga
do you mind reverting the file name refactor? ie updating tests/utils/html_utils.py back to tests/utils/html.py?
ty!

in meantime i will take a look at the test script

value = "ffffff"
if any(x in label for x in bg_labels):
backgrounds[label] = value
else:
Copy link
Owner

Choose a reason for hiding this comment

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

can you explain more what is going on this else block (54-60)? i am a bit lost

how do we end up making use of the color_labels list from matplotlib?
i tried out the Web AIM API with a hex value and one of the matplotlib color names and the result doesn't make sense to me

for example when i compare beige to burlywood

https://webaim.org/resources/contrastchecker/?fcolor=faf0e6&bcolor=burlywood&api

where beige hex is #faf0e6 i get all passes even though by my eye they seem very similar:

{"ratio":"18.4","AA":"pass","AALarge":"pass","AAA":"pass","AAALarge":"pass"}

Copy link
Owner

Choose a reason for hiding this comment

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

comparing beige to the burlywood hexcode #deb887 fails:

https://webaim.org/resources/contrastchecker/?fcolor=faf0e6&bcolor=deb887&api:

{"ratio":"1.65","AA":"fail","AALarge":"fail","AAA":"fail","AAALarge":"fail"}

so if we are using the matplotlib color label i think we need to look up the hex code first
(again, wasn't entirely sure just from reading)

Copy link
Author

Choose a reason for hiding this comment

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

The else block on lines 54-60 is just a basic way to filter for foreground labels or color labels (treated as foreground) within the palettes. This is the only place where the matplotlib color labels are used currently. I just added that as a hacky way to capture additional color labels from palettes, assuming that new palettes could be imported any time.

So you are correct that the review should only be using hex codes and I think that it will only do so as long as the palette css files follow the existing structure (color-name: #HEXVALUE).

Copy link
Author

Choose a reason for hiding this comment

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

Also we can definitely remove the two elif statements if it is decided that all palette css files will follow the same naming convention, using "fg"/"foreground" and "bg"/"background" in the color labels. Other color types (ex: accents) could also be defined as needed.

Right now some palettes may include words like "primary", "secondary", tertiary", "color", or an actual color name (ex: "red") in the label instead of the foreground/background strings, so that is why the elif statements on lines 57-60 are currently used.

@ntno
Copy link
Owner

ntno commented Oct 1, 2024

i like this so far i think it makes sense
i also think if i clean up the existing .css palettes it will work a lot better
for example if i remove the unused colors from gruvbox_dark.css and replace the use of var() we get this output which is much more readable:

gruvbox_dark.css

< <
backgrounds tertiary-color (a89984) secondary-color (bdae93) font-color (ebdbb2) primary-color (fabd2f) error-color (fb4934)
background-color (282828) ☑️ ☑️ ☑️
progress-bar-background (504945) ☑️ ☑️ ☑️ ☑️
code-bg-color (504945) ☑️ ☑️ ☑️ ☑️

@cowellbunga
Copy link
Author

hi @cowellbunga do you mind reverting the file name refactor? ie updating tests/utils/html_utils.py back to tests/utils/html.py? ty!

in meantime i will take a look at the test script

I realized that having the palette review inside of utils caused the utils.html file to clash with an html package (used by palette review) of the same name. Moving the review script outside of the tests.utils package fixes the issue, removing the need for a name change. Name change reverted.

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.

2 participants