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

Dark scheme overlay #7052

Merged
merged 25 commits into from
Oct 24, 2019
Merged

Dark scheme overlay #7052

merged 25 commits into from
Oct 24, 2019

Conversation

Fabianopb
Copy link
Contributor

This PR introduces the dark theme for react-error-overlay!

The theme is inferred in browsers that support the media feature prefers-color-scheme to detect the preferred theme in the user's system. This feature is still cutting-edge and should be released soon in the most used browsers.

Some important notes about this solution:

  • The general idea is similar as in react-devtools, which abstracts all "themeable" variables to a theme object;
  • The theme object is shared throughout react-error-overlay using the Context API;
  • Most components and containers were refactor to allow the use of hooks (mostly useContext).

Tested with Chrome Canary and Firefox Nightly as both already support that. A few screenshots comparing the light and the dark themes:

Build error:
image

Runtime error:
image

Feedback is welcome!

Cheers!
Closes #6760

@Fabianopb
Copy link
Contributor Author

No pressure, just pinging people to have an idea when I would start getting reviews and feedback! So, any idea? :)

@bugzpodder bugzpodder added this to the 3.x milestone Aug 11, 2019
@Fabianopb
Copy link
Contributor Author

Fabianopb commented Aug 13, 2019

It's been a while, any estimations when is this getting to the roadmap?
Ping @iansu or @Timer

@iansu iansu modified the milestones: 3.x, 3.2 Aug 13, 2019
@iansu iansu added this to In progress in v3.3 via automation Aug 13, 2019
@bdefore
Copy link

bdefore commented Oct 20, 2019

I'd vouch that we should only rely on prefers-color-scheme as a default. There are three cases I can think of where this will not be accurate:

  • [minor] Testing pages on unsupported browsers will not work
  • [minor] At this time, not all developers have the ability to change system settings i.e. lack permissions, or ChromeOS
  • A web site under development may have different lighting theme than the developer's system preference

As @iansu has requested this not be exposed as a parameter, perhaps we could have a toggle button (a lightbulb?) on the overlay itself that sets a flag in local storage?

For anyone who's impatient and desires a dirty brittle hack to force dark theme with this branch, at this point in time the following works for me when run from the root of a project that has already run yarn install

DIR=tmp
git clone https://github.com/Fabianopb/create-react-app $DIR
cd $DIR/packages/react-error-overlay
git checkout dark-scheme-overlay
sed -i 's/lightTheme, darkTheme/darkTheme/' src/utils/dom/css.js
sed -i 's/lightTheme/darkTheme/g' src/utils/dom/css.js
yarn install
yarn run build:prod
cd -
mv node_modules/react-error-overlay/lib node_modules/react-error-overlay/lib-pristine
rsync -avm $DIR/packages/react-error-overlay/lib/ node_modules/react-error-overlay/lib/
rm -rf $DIR

@ianschmitz
Copy link
Contributor

@Fabianopb thanks for this! I just realized that we are using hooks in this PR, and unfortunately we are still supporting older versions of React (< 16.8) at this time 😞. Would you be able to refactor so it doesn't rely on hooks?

As @iansu has requested this not be exposed as a parameter, perhaps we could have a toggle button (a lightbulb?) on the overlay itself that sets a flag in local storage?

I like this idea. We could do it in a follow-up PR so we don't hold up this one.

@Fabianopb
Copy link
Contributor Author

@Fabianopb thanks for this! I just realized that we are using hooks in this PR, and unfortunately we are still supporting older versions of React (< 16.8) at this time 😞. Would you be able to refactor so it doesn't rely on hooks?

Hey @ianschmitz it's been a long time! 😅

So yeah it's been a long time and I was trying to remember the reasoning for using hooks (despite of looking cleaner). At that time I probably noticed that React 16.8 was used here, so I thought it would be fine go on with useContext.

I'm seeing now that React is a dev dependency in the error overlay package, so who dictates the version that the overlay uses, is that the app itself?

Well, in that case there's no way around, luckily the useContexts should be quite easy to refactor, and apparently I've introduced only two useEffects.

I can't tell I'll do it very soon as I've been quite busy, but I can take a look within the next few weeks.

@ianschmitz ianschmitz closed this Oct 24, 2019
v3.3 automation moved this from In progress to Done Oct 24, 2019
@ianschmitz ianschmitz reopened this Oct 24, 2019
v3.3 automation moved this from Done to In progress Oct 24, 2019
@ianschmitz
Copy link
Contributor

@Fabianopb sorry yes you're absolutely correct. I overlooked that. We do bundle react in with the overlay bundle. So we're good to use hooks 😄

@ianschmitz ianschmitz merged commit c24314d into facebook:master Oct 24, 2019
v3.3 automation moved this from In progress to Done Oct 24, 2019
@Fabianopb
Copy link
Contributor Author

A W E S O M E

@Fabianopb Fabianopb deleted the dark-scheme-overlay branch October 25, 2019 05:30
@iansu
Copy link
Contributor

iansu commented Oct 25, 2019

Thanks for working on this!

@lock lock bot locked and limited conversation to collaborators Oct 30, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
v3.3
  
Done
Development

Successfully merging this pull request may close these issues.

react-error-overlay dark theme
6 participants