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

Issue#2010 CSS media query prefers-dark-mode #4615

Conversation

bcowgill
Copy link

Requirements

  • Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
  • All new code requires tests to ensure against regressions.

Description of the Change

When you change to dark mode in your OS, your mocha tests in the browser will follow suit.

  1. This fixes an overflow problem when the user expands a single test to see the source code. The code was flowing outside the bounding box. This change now shows a scroll bar the same as for a failed test error box.
  2. This also honours the Operating System color scheme preference for Light or Dark mode using the CSS prefers-color-scheme media query.

Alternate Designs

There is a change needed in the javascript because the progress indicator canvas renders the 100% text in black.

I first tried using CSS only by changing the canvas background to a darker gray which made the text visible but the circle was then surrounded by a gray square in both light and dark modes this worked but looked tacky.

I then tried changing the text color in the javascript to #888 as a middle point between black and white which was ok.

In the end I used the existing two colors for the circle as outline and fill color for the text, which looks pretty good and is consitent with the existing code and colors.

I have tested this on Win10 Edge/Chrome switching the Color setting from control panel.

The file browser-color-scheme-demo.html is for reviewer's convenience to see it working.
Can be removed before final approval.

The files screenshot-mocha-*.png are for reviewer's convenience also, can be kept(moved somewhere) for adding to the documentation or removed before final approval.

I used the CSS prefers-color-scheme implementation because there was a consensus between @steve-taylor and @boneskull on the issue discussion #2747

Why should this be in core?

It's a change to the mocha.css directly and to the color of the rendered progress indicator.

Benefits

  • Users who have set their Operating System color scheme to Dark mode will be able to view their test results in the browser in a dark scheme as well.
  • Users with vision difficulties will be better able to use the browser based test results.
  • When the user views the detailed code for a particular test, the displayed code will have a scroll bar automatically instead of the code overflowing outside the border. (The same as for an error message box.)

Possible Drawbacks

Any further color scheme changes will require dark mode values to be added and tested.
But there hasn't been such a change to the CSS since perhaps 2015 or sooner, the last time one of the color related CSS values changed:

commit c66b34e7e1ee5b9b7054ecccc73f589ead2f8543
Author: Pavel Zubkou <[email protected]>
Date:   Tue Aug 11 17:29:16 2015 +0300

Applicable issues

  • Enter any applicable Issues here.
    Issue#2010

  • Mocha follows semantic versioning: http://semver.org

  • Is this a breaking change (major release)?
    No

  • Is it an enhancement (minor release)?
    Yes

  • Is it a bug fix, or does it not impact production code (patch release)?
    Not a bug fix.

When you change to dark mode in your OS, your mocha tests in the browser will follow suit.

There is a change needed in the javascript because the progress indicator canvas renders the 100% text in black.
I first tried using CSS only by changing the canvas background to a darker gray which made the text visible but the circle was then surrounded by a gray square in both light and dark modes.
I then tried changing the text color in the javascript to mochajs#888 as a middle point between black and white which was ok.
In the end I used the existing two colors for the circle as outline and fill color for the text, which looks pretty good.

I have tested this on Win10 Edge/Chrome switching the Color setting from control panel.

The file browser-color-scheme-demo.html is for reviewer's convenience to see it working.
Can be removed before final approval.

The files screenshot-mocha-*.png are for reviewer's convenience also, can be kept(moved somewhere) for adding to the documentation or removed before final approval.
@bcowgill
Copy link
Author

The screen shot files are not working, adding them here.

screenshot-mocha-light
screenshot-mocha-dark

@bcowgill bcowgill force-pushed the bcowgill/issue/2010-prefers-color-scheme branch from f99b431 to 63c9d26 Compare March 28, 2021 20:16
@bcowgill
Copy link
Author

Unsure why the browser tests fail. I tried reverting my changes on the branch and they still failed.
There must be something wrong with them.

@outsideris
Copy link
Member

Currently, our browser tests can't be run on Actions, see #4553
Sorry about that. You can ignore it if there is no error on your local machine.

@bcowgill
Copy link
Author

bcowgill commented Apr 3, 2021

Thanks, @outsideris, yes the tests run locally with no failures.

@github-actions
Copy link

github-actions bot commented Aug 2, 2021

This PR hasn't had any recent activity, and I'm labeling it stale. Remove the label or comment or this PR will be closed in 14 days. Thanks for contributing to Mocha!

@github-actions github-actions bot added the stale this has been inactive for a while... label Aug 2, 2021
@github-actions github-actions bot closed this Aug 18, 2021
@JoshuaKGoldberg
Copy link
Member

👋 coming back to this @bcowgill, are you still interested in working on this PR? As of #5027 there are again maintainers who can review it now. No worries if you no longer have time - but if you do that'd be great!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale this has been inactive for a while...
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants