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

Created dark theme #330

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

Created dark theme #330

wants to merge 3 commits into from

Conversation

HeadMerchant
Copy link
Collaborator

  • Added dark color scheme togglable through button in MainPage.vue
    Screen Shot 2019-10-27 at 2 50 51 AM
    Screen Shot 2019-10-27 at 2 52 04 AM
  • Theme colors available in app.js
  • Primary color in dark theme is "discord orange"
  • Changed cookie banner color to "discord purple"
  • Semester status colors when adding classes are now more intense
  • Dark theme uses Halloween colors :)
    Screen Shot 2019-10-27 at 2 50 41 AM
  • Dark state is managed in the Vuex store
  • Theme colors are accessible via "this.$vuetify.theme."...
  • Color variants are toggled in the setter for "useDark" in MainPage

- Added dark color scheme togglable through button in MainPage.vue
- Theme colors available in app.js
- Primary color in dark theme is "discord orange"
- Changed cookie banner color to "discord purple"
- Semester status colors when adding classes are now more intense
- Dark theme uses Halloween colors :)
- Dark state is managed in the Vuex store
- Theme colors are accessible via this.$vuetify.theme...
- Color variants are toggled in the setter for "useDark" in MainPage
Copy link
Collaborator

@npfoss npfoss left a comment

Choose a reason for hiding this comment

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

Haven't finished reviewing but here are some initial thoughts on the theme in particular.

This is looking great, just a few nits on structure

@@ -2,6 +2,7 @@
<v-app
id="app-wrapper"
>
<link href="https://cdn.jsdelivr.net/npm/@mdi/[email protected]/css/materialdesignicons.min.css" rel="stylesheet">
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think these should already be bundled with the app? Definitely check out how other icons are done. We shouldn't need to import more

src/app.js Outdated
Comment on lines 22 to 42
Vue.use(Vuetify, {
theme: {
primary: "#F26521",
defaultPrimary: "#1976D2",
discordOrange: "#F26521",
discordPurple: "#7289DA",
crlogo: colors.blueGrey.darken2,
crlogoDark: colors.blueGrey.darken2,
crlogoLight: colors.blueGrey.lighten5,
background: colors.grey.darken4,
background2: colors.grey.darken4,
search: colors.grey.darken4,
backgroundDark: colors.grey.darken4,
backgroundLight: colors.grey.lighten2,
background2Light: colors.grey.lighten4,
searchLight: colors.shades.white,
warning: colors.yellow.base,
success: colors.green.base,
error: colors.red.base
}
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need to override all of these or are some the defaults?

crlogoDark: colors.blueGrey.darken2,
crlogoLight: colors.blueGrey.lighten5,
background: colors.grey.darken4,
background2: colors.grey.darken4,
Copy link
Collaborator

Choose a reason for hiding this comment

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

no need to duplicate these two?

@@ -18,7 +19,27 @@ library.add(faSignInAlt, faSignOutAlt, faCloudDownloadAlt, faCloudUploadAlt);

Vue.component('font-awesome-icon', FontAwesomeIcon);

Vue.use(Vuetify);
Vue.use(Vuetify, {
theme: {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it might be better to keep the theme general, so like backgroundDark rather than singling out specific use cases here, like search. This way if you're reading the search code you can see and change the color there without having to come here, and the very high level section here isn't polluted with use-case-specific color settings

search: colors.grey.darken4,
backgroundDark: colors.grey.darken4,
backgroundLight: colors.grey.lighten2,
background2Light: colors.grey.lighten4,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are there not already colors like this as part of the Vuetify default theme? I feel like we should just be able to use what's already there, and override that if it's not sufficient.

@npfoss
Copy link
Collaborator

npfoss commented Oct 31, 2019

Also, like I said in #courseroad-dev but repeating here for the permanent record, I'd darken the background colors a bit in dark mode, so something like #f1c400 for the yellow maybe.

Copy link
Collaborator

@npfoss npfoss left a comment

Choose a reason for hiding this comment

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

Some more thoughts on how to structure the theme stuff:

There are a lot of conditionals for colors based on the darkness setting, but there's a way to do that within the theme so you can put it all in one place. See the [Vuetify page](See this example: https://vuetifyjs.com/en/customization/theme) for all the details, but long story short you should be able to do something like this:

const vuetify = new Vuetify({
  theme: {
    themes: {
      light: {
        primary: colors.purple,
      },
      dark: {
        primary: colors.blue.lighten3,
      },
    },
  },
})

Comment on lines +274 to +281
set (value) {
this.$store.commit('setUseDarkTheme', value);
this.$vuetify.theme.primary = value ? this.$vuetify.theme.discordOrange : this.$vuetify.theme.defaultPrimary;
this.$vuetify.theme.background = value ? this.$vuetify.theme.backgroundDark : this.$vuetify.theme.backgroundLight;
this.$vuetify.theme.background2 = value ? this.$vuetify.theme.backgroundDark : this.$vuetify.theme.background2Light;
this.$vuetify.theme.search = value ? this.$vuetify.theme.backgroundDark : this.$vuetify.theme.searchLight;
this.$vuetify.theme.crlogo = value ? this.$vuetify.theme.crlogoDark : this.$vuetify.theme.crlogoLight;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm pretty sure this whole block can be handled just by setting different themes for dark and light mode in the initial theme setup

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.

2 participants