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

Chore: change dark mode to use Bootstrap's color modes #4174

Merged

Conversation

lkster
Copy link
Contributor

@lkster lkster commented Sep 12, 2023

Proposed change

I'm working on custom theme for paperless and needed to refactor this anyway so maybe it would be actually worth to add.

Currently paperless has it's own mechanism to set specific theme. Bootstrap 5.3 implemented color modes functionality which is then used for dark mode. Since paperless uses Bootstrap, it'd be good to actually utilize this functionality for dark mode.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Other (please explain): refactor

Checklist:

  • I have read & agree with the contributing guidelines.
  • If applicable, I have included testing coverage for new code in this PR, for backend and / or front-end changes.
  • If applicable, I have tested my code for new features & regressions on both mobile & desktop devices, using the latest version of major browsers.
  • If applicable, I have checked that all tests pass, see documentation.
  • I have run all pre-commit hooks, see documentation.
  • I have made corresponding changes to the documentation as needed.
  • I have checked my modifications for any breaking changes.

@lkster lkster requested a review from a team as a code owner September 12, 2023 21:52
@paperless-ngx-secretary paperless-ngx-secretary bot added frontend non-trivial Requires approval by several team members labels Sep 12, 2023
@paperless-ngx-secretary
Copy link

Hello @ThaFog,

thank you very much for submitting this PR to us!

This is what will happen next:

  1. My robotic colleagues will check your changes to see if they break anything. You can see the progress below.
  2. Once that is finished, human contributors from paperless-ngx review your changes. Since this is a non-trivial change, a review from at least two contributors is required.
  3. Please improve anything that comes up during the review until your pull request gets approved.
  4. Your pull request will be merged into the dev branch. Changes there will be tested further.
  5. Eventually, changes from you and other contributors will be merged into main and a new release will be made.

Please allow up to 7 days for an initial review. We're all very excited about new pull requests but we only do this as a hobby.
If any action will be required by you, please reply within a month.

@shamoon
Copy link
Member

shamoon commented Sep 12, 2023

Can I ask what the real advantage is here, besides being more bootstrap-y? For the most part I see almost no difference except brighter borders (which I dont think we should change) and some visual bugs (see below), and its not like this reduces our codebase or anything (does it)?

Screenshot 2023-09-12 at 2 59 09 PM

And curious what you had in mind with a 'theme'?

@shamoon shamoon changed the title Dark mode based on Bootstrap's color modes Chore: change dark mode to use Bootstrap's color modes Sep 12, 2023
@shamoon
Copy link
Member

shamoon commented Sep 12, 2023

Edit: I fixed the couple small things I noticed. In general Im OK with this but yea, curious about what the perceived advantage is & your theme idea

@lkster
Copy link
Contributor Author

lkster commented Sep 13, 2023

With only this there's barely any advantage other than utilizing bs feature. However there should be after bigger theming refactor (eg. sticking more to bootstrap's variables) so I'd treat it as some step towards it. This is at least a starting point for me. As I said, I felt like it's good to PR this here as I've done this anyway. Next work I guess will be more related to refactoring and changing theme.

About the theme itself - in general I try to unify look of every app I have on my personal homeserver wherever this is possible. My styling is also based on bootstrap which you can find here: https://bootstrap.lukester.app/ . So in the end the transition should be easier + if you like it in the end, I'd be happy to share it.

Btw. sorry for these borders, I was testing it on fresh instance

@shamoon shamoon force-pushed the feat/bootstrap-dark-color-mode-support branch from 872d0cc to b9e5e03 Compare September 13, 2023 02:59
@codecov
Copy link

codecov bot commented Sep 13, 2023

Codecov Report

Merging #4174 (cf8a4da) into dev (d1292c5) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

❗ Current head cf8a4da differs from pull request most recent head 492dece. Consider uploading reports for the commit 492dece to get more accurate results

@@            Coverage Diff             @@
##              dev    #4174      +/-   ##
==========================================
- Coverage   95.36%   95.36%   -0.01%     
==========================================
  Files         339      339              
  Lines       12892    12889       -3     
  Branches     1060     1058       -2     
==========================================
- Hits        12295    12292       -3     
  Misses        592      592              
  Partials        5        5              
Flag Coverage Δ
backend 94.17% <ø> (ø)
frontend 96.78% <100.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
src-ui/src/app/services/settings.service.ts 87.91% <100.00%> (-0.20%) ⬇️

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@shamoon shamoon force-pushed the feat/bootstrap-dark-color-mode-support branch from b9e5e03 to bbbe572 Compare September 13, 2023 03:07
@shamoon
Copy link
Member

shamoon commented Sep 13, 2023

Yea, so this also broke the 'live' application of toggling "use system settings". I've simplified things, I think we really only need 3 cases: "use system settings" = "auto" otherwise dark or light.

Let me know if you have any issues, Im ok to merge this (a lot of work to arrive back where things are currently =), if you have larger ideas about themeing please open a discussion or find us in the chat.

@shamoon shamoon enabled auto-merge (squash) September 13, 2023 03:09
@shamoon shamoon added this to the v2.0.0 milestone Sep 13, 2023
@lkster
Copy link
Contributor Author

lkster commented Sep 13, 2023

Huh sorry once again for that, I see it was indeed quick testing on my side. Anyway sure I can show you everything when I'm done so we'll be able to discuss the final direction if it turns out to be going into official repo

@shamoon shamoon force-pushed the feat/bootstrap-dark-color-mode-support branch from cf8a4da to 492dece Compare September 13, 2023 18:02
@shamoon shamoon merged commit 78ae4c4 into paperless-ngx:dev Sep 13, 2023
11 checks passed
shamoon added a commit that referenced this pull request Oct 13, 2023
* Change setting dark mode to use Bootstrap's data-bs-theme attribute

* Update dark mode styling to use Bootstrap's color mode attribute

* Update unit tests and lints

* Fix not reflecting custom theme color

* Remove commented-out code

* fix inverted thumbnails in dark mode & card borders

* prettier

* Fix application of dark mode, tests

---------

Co-authored-by: shamoon <4887959+shamoon@users.noreply.github.com>
@github-actions
Copy link
Contributor

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new discussion or issue for related concerns.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
frontend non-trivial Requires approval by several team members
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants