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

Added Darkmode #932

Merged
merged 13 commits into from
Aug 3, 2022
Merged

Added Darkmode #932

merged 13 commits into from
Aug 3, 2022

Conversation

narickmann
Copy link
Contributor

Added button for darkmode in Studio (on settings-page).
Studio also detects OS preference (auto-detection) and launches studio in dark- or lightmode.

NOT tested with MacOS

Auto-detection does not work with Chrome and Chromium on Fedora 35, this seems to be a bug.

Added button for darkmode in Studio (on settings-page).
Studio also detects OS preference (auto-detection) and launches studio in dark- or lightmode.

NOT tested with MacOS

Auto-detection does not work with Chrome and Chromium on Fedora 35, this seems to be a bug.
@github-actions
Copy link

This pull request has conflicts ☹
Please resolve those so we can review the pull request.
Thanks.

@github-actions github-actions bot added the status:conflicts Conflicts with another pull request or issue label May 30, 2022
@github-actions github-actions bot removed the status:conflicts Conflicts with another pull request or issue label May 30, 2022
Fixed typo
Copy link
Member

@LukasKalbertodt LukasKalbertodt left a comment

Choose a reason for hiding this comment

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

It's a good start, but I found a few bugs and suboptimal things:

Major:

  • When first loading (without having set a mode, e.g. try incognito mode), the header is wrong. Tried this on windows+ubuntu with chrome+firefox. (Edit: done ✔️)
    image

  • Similarly, when recording in darkmode, the header overlay should be dark, not white: (Edit: done ✔️)
    image

Minor:

  • The auto detection of dark mode did not work for me at all, again, tested on windows+ubuntu (both set to dark mode) with chrome+firefox. But I think it never worked for me on any website at all. So maybe it's me 🤷
  • When video source is still loading, the big loading item has no visible shadow, so it blends right into the background, making the settings icon float weirdly in one random spot. (Edit: done ✔️)
  • In the video settings buttons, the selected button (white bg) has fairly poor contrast with the text
  • All error messages (e.g. try declining camera access in the browser) have black text in dark mode, but the text should be white(-ish) (Edit: done ✔️)
  • I saw one additional case of poor contrast, but can't find it anymore °_°

src/i18n/locales/de.json Outdated Show resolved Hide resolved
src/i18n/locales/de.json Outdated Show resolved Hide resolved
src/App.js Outdated Show resolved Hide resolved
src/App.js Outdated Show resolved Hide resolved
src/ui/header.js Outdated Show resolved Hide resolved
src/ui/settings/colorMode.js Outdated Show resolved Hide resolved
src/ui/settings/colorMode.js Outdated Show resolved Hide resolved
src/ui/studio/review/index.js Outdated Show resolved Hide resolved
src/ui/studio/review/index.js Outdated Show resolved Hide resolved
src/ui/studio/review/index.js Outdated Show resolved Hide resolved
- header has now correct color on initial loading
- when recording the header overlay is now darker in darkmode
- error messages: text now white/light in darkmode
- removed accidentally added whitepaces (my IDE had "format on save" active)
- other minor things [e.g. removed accidentally added whitepaces (my IDE had "format on save" active)]
Copy link
Member

@LukasKalbertodt LukasKalbertodt left a comment

Choose a reason for hiding this comment

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

That solves most of problems! I resolved my github comments where appropriate and updated my last comment.
I found a few minor things in the new change, plus there are a few things from the initial review left.

src/ui/settings/colorMode.js Outdated Show resolved Hide resolved
src/ui/settings/colorMode.js Outdated Show resolved Hide resolved
src/ui/settings/colorMode.js Outdated Show resolved Hide resolved
@lkiesow
Copy link
Contributor

lkiesow commented May 31, 2022

The auto detection of dark mode did not work for me at all, again, tested on windows+ubuntu (both set to dark mode) with chrome+firefox. But I think it never worked for me on any website at all. So maybe it's me shrug

It's not just you. It usually works for me, but it did not when I entered the test deployment. It did after I cleared my local storage. The reason is very likely that on the old studio, you already have theme-ui-color-mode:"default" in the local storage. The new code seems to interpret that as “light” and then store this as a fixed choice.

This also means that if you never set anything, but change your desktop color mode, the “auto” mode will fail to engage. It's just engaging the first time you ever enter Studio.

Having the choice between “light”, “dark” and “auto” with “auto” being the default would be better, I think.

Screenshot from 2022-05-31 21-42-22

Apart from that, I find the lonely green button in the settings weird. I wouldn't expect this color or form for a toggle. If it's a toggle, I would expect something like the preview mode toggle in the editor:

Screenshot from 2022-05-31 21-45-54

But I would probably prefer a dropdown (since it's already used on this page). Though radio buttons would technically also work.

@lkiesow
Copy link
Contributor

lkiesow commented May 31, 2022

The red button and green tooltip colors clash horribly:
Screenshot from 2022-05-31 21-51-29

You turned the play button green. Why? It's also inconsistent with nut turning other icon based buttons green:

Screenshot from 2022-05-31 21-53-11

Screenshot from 2022-05-31 21-52-48

The filled, white pause button is very bright and immediately catches your eye, despite not being the most important button. For other buttons, you used just a white outline, maybe that would work better here as well. Also, I'm wondering if filling the record button white or light gray would beworth a shot:

Screenshot from 2022-05-31 21-51-39
.

@github-actions
Copy link

github-actions bot commented Jun 1, 2022

This pull request has conflicts ☹
Please resolve those so we can review the pull request.
Thanks.

@github-actions github-actions bot added the status:conflicts Conflicts with another pull request or issue label Jun 1, 2022
@LukasKalbertodt
Copy link
Member

(sorry, the conflict was caused by me merging dependabot PRs; I thought that wouldn't cause any conflict. But it's an easy to fix conflict)

@github-actions github-actions bot removed the status:conflicts Conflicts with another pull request or issue label Jun 2, 2022
naweidner added 4 commits June 2, 2022 12:33
- removed button for darkmode and added a dropdowm instead
- updated theme-ui to v0.14.5 this closes #812
- added some config -> this plus update fixes bug with auto preference for darkmode
- some color adjustments, also removed some unused colors from theme.js
- moved css for tooltip and tab-navigation to theme.js (they now use color variables)
- Studio was always choosing system-preference over localstorage, so if the user set Studio to darkmode but system is in lightmode, on refresh Studio was always using lightmode
- So now: on initial load Studio uses system-preference (if nothing stored to localstorage) otherwise Studio uses localstorage
- changed the focus-colors for tab-navigation for better contrast in light and darkmode
- focus-outline for recording buttons now round
- changed outline-color for video-settings to green for better visibility
- aligned outline on recording-buttons
@narickmann
Copy link
Contributor Author

Commit 435cac1 fixes PR #940

1 similar comment
@narickmann
Copy link
Contributor Author

Commit 435cac1 fixes PR #940

Copy link
Member

@LukasKalbertodt LukasKalbertodt left a comment

Choose a reason for hiding this comment

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

I have only skimmed the new changes, but from testing the deployment, I'm completely happy with it!

naweidner added 2 commits June 30, 2022 14:47
System preference should now work. After OS preferences have been changed, the page must be reloaded to see the change in Studio.
Copy link
Member

@LukasKalbertodt LukasKalbertodt left a comment

Choose a reason for hiding this comment

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

Just some comments on the code regarding the last two commits. Otherwise, as I said, I'm happy and would like to merge.

@lkiesow Could you take a look again to make sure the points raised in your last comment are all fixed? They look fixed to me.

package.json Outdated Show resolved Hide resolved
src/ui/settings/colorMode.js Outdated Show resolved Hide resolved
src/ui/settings/colorMode.js Outdated Show resolved Hide resolved
src/ui/settings/colorMode.js Outdated Show resolved Hide resolved
@github-actions github-actions bot added the status:conflicts Conflicts with another pull request or issue label Aug 1, 2022
@github-actions

This comment was marked as resolved.

@github-actions github-actions bot removed the status:conflicts Conflicts with another pull request or issue label Aug 2, 2022
- removed unused package
- indented a few lines
Copy link
Member

@LukasKalbertodt LukasKalbertodt left a comment

Choose a reason for hiding this comment

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

Yay!

@LukasKalbertodt LukasKalbertodt merged commit df467fb into elan-ev:master Aug 3, 2022
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.

3 participants