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

feat(ui): updates to dark mode logic #3740

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

zackbcom
Copy link
Contributor

@zackbcom zackbcom commented Jun 2, 2024

Changes

  • Updates to dark mode logic to have 3 options, System defined, Dark and, Light.
  • Reused bool in the settings to use null as system defined.
  • Created listener for changes to system dark mode. Works on mac when selecting system theme.
  • Added button toggle to support the 3 cases.

TODO

  • Need to figure out what to do with the login checkbox.
    • Might be able to look into a indeterminate state for system.
    • Replace with button stack
    • Remove and use system default, update in settings.

Testing Artifacts

  • After
    • image

@coveralls
Copy link

coveralls commented Jun 3, 2024

Pull Request Test Coverage Report for Build 9362246394

Details

  • 0 of 1 (0.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 21.163%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/stores/base.js 0 1 0.0%
Totals Coverage Status
Change from base Build 9347582370: 0.0%
Covered Lines: 3899
Relevant Lines: 19593

💛 - Coveralls

@@ -666,6 +669,8 @@ export default {
hideTopbar: false,
title: '',
messages: [],
systemDark: false,
mq_system_dark: null,
Copy link
Member

Choose a reason for hiding this comment

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

why don't use a watch for this instead?

Copy link
Member

Choose a reason for hiding this comment

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

Also it's not clear to me what mq_system_dark is used for, you never re-assign it, I would instead use a let in beforeMount instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

First time using Vue. Will re look at the logic here. Thanks for the notes.

Copy link
Member

Choose a reason for hiding this comment

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

It's ok no worries I appreciate a lot your help 🙏🏼

Comment on lines +1632 to +1639
beforeUnmount() {
if (this.mq_system_dark !== null) {
this.mq_system_dark.removeEventListener(
'change',
this.updateSystemDark,
)
}
},
Copy link
Member

Choose a reason for hiding this comment

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

this is a bit useless as the App will only be unmounted when the page is destroyed

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.

None yet

3 participants