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(dark-theme): add dark theme colors #391

Merged
merged 3 commits into from
Jan 9, 2021
Merged

feat(dark-theme): add dark theme colors #391

merged 3 commits into from
Jan 9, 2021

Conversation

heyhippari
Copy link
Contributor

@heyhippari heyhippari commented Dec 15, 2020

  • Sets a bunch of colors for the dark Vuetify theme
  • Moves some menus and buttons around, adds tooltips
  • Improve readability of the app bar buttons when the bar is transparent, by using floating action buttons

Updated 2020/01/08
image

@heyhippari heyhippari marked this pull request as draft January 3, 2021 02:24
@codecov-io
Copy link

codecov-io commented Jan 3, 2021

Codecov Report

Attention: Patch coverage is 0% with 35 lines in your changes missing coverage. Please review.

Project coverage is 0.59%. Comparing base (28e351c) to head (4d385dc).
Report is 3782 commits behind head on master.

Files with missing lines Patch % Lines
components/Layout/HomeHeader/HomeHeaderItems.vue 0.00% 13 Missing ⚠️
components/Buttons/UserButton.vue 0.00% 11 Missing ⚠️
pages/artist/_itemId/index.vue 0.00% 5 Missing ⚠️
pages/item/_itemId/index.vue 0.00% 5 Missing ⚠️
components/Item/Card.vue 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff            @@
##           master    #391      +/-   ##
=========================================
- Coverage    0.60%   0.59%   -0.01%     
=========================================
  Files          88      88              
  Lines        2312    2344      +32     
  Branches      353     355       +2     
=========================================
  Hits           14      14              
- Misses       2297    2329      +32     
  Partials        1       1              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@heyhippari heyhippari marked this pull request as ready for review January 8, 2021 01:28
@heyhippari
Copy link
Contributor Author

This should be pretty much ready for review. Though it only handles the dark theme.

We'll have to see if we want t specific color scheme for the light theme or not. I'm thinking of going for something with the same general hue/saturation as the dark one, but a lot lighter, if only to get away from pure white.

@ThibaultNocchi
Copy link
Member

I agree that pure white can be jarring, but Github for instance has a pure white background. But to complement it there are dark accents, for instance on the top bar.

Copy link
Contributor

@camc314 camc314 left a comment

Choose a reason for hiding this comment

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

Looks good so far, few comments though.

Spacing is off in user button

image

components/Buttons/UserButton.vue Show resolved Hide resolved
components/Buttons/UserButton.vue Show resolved Hide resolved
components/System/LocaleSwitcher.vue Outdated Show resolved Hide resolved
Copy link
Member

@ferferga ferferga left a comment

Choose a reason for hiding this comment

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

This branch somehow uses Roboto instead of Noto.

Imports needs to happen in this order: I tried adding the fontsource imports to variables.scss but it seems that they got imported for every single component, resulting in a bundle size over 100 MB.

assets/variables.scss Outdated Show resolved Hide resolved
assets/variables.scss Outdated Show resolved Hide resolved
@sonarqubecloud
Copy link

sonarqubecloud bot commented Jan 9, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@heyhippari
Copy link
Contributor Author

Imports needs to happen in this order

We apparently don't need to import the style in the variables at all :) I reworked the file based on this: https://github.com/nuxt-community/vuetify-module#customvariables

Not including the import at the end works properly, so I left it out.

Copy link
Member

@ferferga ferferga left a comment

Choose a reason for hiding this comment

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

LGTM now, nothing else to comment aside from what I mention here

@heyhippari heyhippari merged commit 9793c5f into master Jan 9, 2021
@heyhippari heyhippari deleted the theme-colors branch January 9, 2021 20:43
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.

5 participants