Skip to content

dark mode #381

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

Closed
wants to merge 1 commit into from
Closed

dark mode #381

wants to merge 1 commit into from

Conversation

jkowalleck
Copy link
Contributor

@jkowalleck jkowalleck commented Nov 26, 2020

users that have their OS/Browsers set to dark mode expect to see web sites in a darker tone.
so this PR will add a darker tone to re resulting report.

resulting HTML is this: https://jkowalleck.github.io/pytest-html/report.html
used .tox/devel/log/report.html after tox -e devel
image as shown here: #381 (comment)

@codecov-io
Copy link

codecov-io commented Nov 26, 2020

Codecov Report

Merging #381 (e2ce4e9) into master (67d2135) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #381   +/-   ##
=======================================
  Coverage   97.63%   97.63%           
=======================================
  Files           4        4           
  Lines         423      423           
=======================================
  Hits          413      413           
  Misses         10       10           
Flag Coverage Δ
tests 97.63% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 67d2135...e2ce4e9. Read the comment docs.

@BeyondEvil
Copy link
Contributor

BeyondEvil commented Nov 26, 2020

Hey @jkowalleck ,

Thanks for the PR! 🙏

Would you mind adding the report or screenshot of the report as a file attached to this PR? So that the link doesn't break in the future if it's removed. 😊

Cheers

@@ -28,6 +29,25 @@ table {
border-collapse: collapse;
}

@media (prefers-color-scheme: dark) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What does the browser support look like for this annotation? Will this break older browsers?

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems pretty recent: https://developer.mozilla.org/en-US/docs/Web/CSS/@media/prefers-color-scheme

How will older browsers handle this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

general browser support: https://caniuse.com/mdn-css_at-rules_media
special browser support: https://caniuse.com/prefers-color-scheme

old browsers will not be broken. they will simply ignore it

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice! Thanks!

@jkowalleck
Copy link
Contributor Author

Hello @BeyondEvil

Would you mind adding the report or screenshot of the report as a file attached to this PR? So that the link doesn't break in the future if it's removed.

resulting HTML is this: https://jkowalleck.github.io/pytest-html/report.html
used .tox/devel/log/report.html after tox -e devel

here is a screenshot, for those that dont have dark mode enabled: https://jkowalleck.github.io/pytest-html/report.png

@BeyondEvil
Copy link
Contributor

BeyondEvil commented Nov 26, 2020

Image of result for record keeping

darkmodereport

This is what I meant. 😊

BeyondEvil
BeyondEvil previously approved these changes Nov 26, 2020
Copy link
Contributor

@BeyondEvil BeyondEvil left a comment

Choose a reason for hiding this comment

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

Looks good to me, but I'm no CSS ace. @gnikonorov @ssbarnea any comments?

@BeyondEvil
Copy link
Contributor

Is it possible to (unit) test this somehow? 🤔

@jkowalleck
Copy link
Contributor Author

Looks good to me, but I'm no CSS ace. @gnikonorov @ssbarnea any comments?

thanks :-)

i loved to have the colors written differently. but since there was no sass/scss and colors were all over the place, i did what i could.

i would suggest to strip the colors from where they are and have default- and dark-theme coloring in an extra section ...
but basically its up to you.
just write me a message if you want something changed. or create a PR to my branch,

@BeyondEvil
Copy link
Contributor

i loved to have the colors written differently. but since there was no sass/scss and colors were all over the place, i did what i could.

i would suggest to strip the colors from where they are and have default- and dark-theme coloring in an extra section ...
but basically its up to you.
just write me a message if you want something changed. or create a PR to my branch,

Is it a lot of work to unify the colors in it's own (I guess reusable?) section? @jkowalleck

@jkowalleck
Copy link
Contributor Author

i loved to have the colors written differently. but since there was no sass/scss and colors were all over the place, i did what i could.
i would suggest to strip the colors from where they are and have default- and dark-theme coloring in an extra section ...
but basically its up to you.
just write me a message if you want something changed. or create a PR to my branch,

Is it a lot of work to unify the colors in it's own (I guess reusable?) section? @jkowalleck

not much.
will write an extra PR on top of this one, that does it, so you can merge it if you like.

Copy link
Member

@gnikonorov gnikonorov left a comment

Choose a reason for hiding this comment

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

Thanks for this @jkowalleck!

Can you also add a changelog entry? You can pretty much copy the changes to CHANGES.rst in #380, just replace the description and authors with a description of this change and yourself.

@gnikonorov
Copy link
Member

Looks good to me, but I'm no CSS ace. @gnikonorov @ssbarnea any comments?

I can take a thorough look at this tonight/tomorrow @BeyondEvil

@jkowalleck
Copy link
Contributor Author

Thanks for this @jkowalleck!

Can you also add a changelog entry? You can pretty much copy the changes to CHANGES.rst in #380, just replace the description and authors with a description of this change and yourself.

done that.

@jkowalleck
Copy link
Contributor Author

closed in favor of #383.

@jkowalleck jkowalleck closed this Nov 27, 2020
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.

4 participants