[PREVIEW] Dark mode and scss #383
Conversation
|
Thanks @jkowalleck, this is awesome! Does that mean we can close #381 in favor of this one? I'm all in favor for switching to SCSS while introducing dark mode. I was going to ask you to raise a separate issue for it originally |
i think so. If this PR is good enough, i would be happy if #381 was closed. |
|
Let's close it then. Unless @BeyondEvil has an issue with us moving to SCSS while we do this |
yes, @BeyondEvil. if this very PR is fine, i will discard #381, |
|
|
||
| * Thanks to `@jkowalleck <https://github.com/jkowalleck>`_ for the PR | ||
|
|
||
| * Support for dark mode (`#381 <https://github.com/pytest-dev/pytest-html/issues/381>`_) |
There was a problem hiding this comment.
🔧 need to remove this, when #381 is closed
| &.skipped, | ||
| &.xfailed, | ||
| &.rerun { | ||
| font-weight: bold; |
There was a problem hiding this comment.
this is new,
i fealed like i needed it bold for better readability.
| } | ||
| } | ||
|
|
||
| .col-result { |
There was a problem hiding this comment.
this is new,
i fealed like i needed it bold it for better readability.
Codecov Report
@@ Coverage Diff @@
## master #383 +/- ##
=======================================
Coverage 97.63% 97.63%
=======================================
Files 4 4
Lines 423 423
=======================================
Hits 413 413
Misses 10 10
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
* created a folder `src` and moved the existing `pytest_html` into it. * transferred `.../resources/style.css` to scss and had it split. als added some variables. * made the colors applied via a scss mixin for light- and dark-theme.
There was a problem hiding this comment.
I am in favor of the proposed change, likely all parts of it. There is only one problem: it does combine unrelated changes, and major ones.
For example, I am really happy to see the move of module inside src/ folder, which is following the recommended practices. Still, this rename should be done without touching anything else.
The same can be said about adoption of scss, which is ok by its own but not combined with addition of another mode.
Changes should be atomic, implementing only one feature at a time. Otherwise we risk introducing too many bugs and doing a poor job reviewing it.
My proposal: keep this open but make another change that is doing only the src move. Once we merge you can rebase and the size will go down.
❤️ i agree with you, @ssbarnea. Created a Create another PR #385 on top of #384, that introduces use of SASS/SCSS as a atomic change. Will create another PR on top, that adds dark mode as a feature. |
|
SCSS was integrated as needed into |
As stated via a comment in #381:
here is a proposal how consolidating the colors in a reusable way may look like.
A comment in #381 made me feel like old browsers need support.
Using css variables was not a choice, since
this PR's solution has some notable impact:
style.cssis now based on SASS/SCSS that is split into logical modules.editing the css is quite easy. Its SCSS now, which is pretty similar to the previous CSS.
generating the
style.cssfrom SCSS/SASS was made aeasy via commandnpm run build-css.pytest_htmlwas moved down to foldersrc. for easy overview.This was done, since the layout is now a SASS/SCSS that generates code. To have developers aware of this, the additional SASS/SCSS files were moved in a place, where they can be seen easily as code.
Furthermore, since the generated code is compressed, a frontend-developper will see where it comes from just by traversing the path to the generated css.
style.cssis compressed/minified, now. its not human-readable anymore.i think this is a good decision. package becomes smaller, output from
pytest_htmlbecomes smaller.