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

[Enhancement] Theme: Midnight #2312

Merged
merged 17 commits into from
Feb 16, 2024
Merged

Conversation

shall0pass
Copy link
Contributor

@shall0pass shall0pass commented Feb 1, 2024

There are a few places I wanted a slightly different shade, but two different components are using the same variable.

Feel free to push PR's to this branch, especially if you know exactly what you want changed.

There's also a bug I wasn't able to sort out. The theme choice doesn't seem to be saved as a global pref. I'm not sure what's overriding it. At one point I wrote out to the console every time the useTheme() function was called. Most pages (reports, payees, schedules, etc) called the function once, but the settings page calls it 5 times. The first two calls it gets the correct theme, but by the time the 3rd call is made the theme has been changed to 'light' and that's the end result.

I left a few comments in place to remind me of what changes what. Unfortunately, I started commenting late in the process so there is a lot missing. They can be removed when everyone's happy with it.

Preview images
image
image
image
image
image

Copy link

netlify bot commented Feb 1, 2024

Deploy Preview for actualbudget ready!

Name Link
🔨 Latest commit 5602b62
🔍 Latest deploy log https://app.netlify.com/sites/actualbudget/deploys/65ccf6e079450f00082f1904
😎 Deploy Preview https://deploy-preview-2312.demo.actualbudget.org/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@trafico-bot trafico-bot bot added the 🚧 WIP label Feb 1, 2024
Copy link
Contributor

github-actions bot commented Feb 1, 2024

Bundle Stats — desktop-client

Hey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle.

As this PR is updated, I'll keep you updated on how the bundle size is impacted.

Total

Files count Total bundle size % Changed
10 4.44 MB → 4.45 MB (+7.21 kB) +0.16%
Changeset
File Δ Size
src/style/themes/midnight.ts 🆕 +6.54 kB 0 B → 6.54 kB
src/style/palette.ts 📈 +176 B (+13.64%) 1.26 kB → 1.43 kB
src/style/themes/light.ts 📈 +374 B (+5.59%) 6.54 kB → 6.9 kB
src/style/colors.ts 📈 +44 B (+3.83%) 1.12 kB → 1.17 kB
src/style/theme.tsx 📈 +67 B (+3.64%) 1.8 kB → 1.86 kB
src/components/ThemeSelector.tsx 📈 +28 B (+2.35%) 1.16 kB → 1.19 kB
View detailed bundle breakdown

Added

No assets were added

Removed

No assets were removed

Bigger

Asset File Size % Changed
static/js/index.js 2.73 MB → 2.74 MB (+7.21 kB) +0.26%

Smaller

No assets were smaller

Unchanged

Asset File Size % Changed
static/js/indexeddb-main-thread-worker-e59fee74.js 13.5 kB 0%
static/js/resize-observer.js 18.37 kB 0%
static/js/ButtonLink.js 379 B 0%
static/js/BackgroundImage.js 122.29 kB 0%
static/js/narrow.js 79.83 kB 0%
static/js/BalanceTooltip.js 6.06 kB 0%
static/js/MenuTooltip.js 20 kB 0%
static/js/wide.js 267.28 kB 0%
static/js/ReportRouter.js 1.2 MB 0%

Copy link
Contributor

github-actions bot commented Feb 1, 2024

Bundle Stats — loot-core

Hey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle.

As this PR is updated, I'll keep you updated on how the bundle size is impacted.

Total

Files count Total bundle size % Changed
1 1.2 MB → 1.2 MB (+35 B) +0.00%
Changeset
File Δ Size
packages/loot-core/src/server/main.ts 📈 +51 B (+0.08%) 65.62 kB → 65.67 kB
View detailed bundle breakdown

Added

No assets were added

Removed

No assets were removed

Bigger

Asset File Size % Changed
kcab.worker.js 1.2 MB → 1.2 MB (+35 B) +0.00%

Smaller

No assets were smaller

Unchanged

No assets were unchanged

@youngcw
Copy link
Member

youngcw commented Feb 1, 2024

Nice. I feel like the background color should be darker but everything else looks good

@shall0pass
Copy link
Contributor Author

Currently the modal border property is commented out. If we could enable that property again, it may help with the visuals of the pop up modals against the rest of the app, even if the modals have similar shades to the base app.

@shall0pass shall0pass changed the title [WIP] Theme: Midnight [Enhancement] Theme: Midnight Feb 2, 2024
@youngcw
Copy link
Member

youngcw commented Feb 2, 2024

The color of the upcoming schedule pill seems too light. I think the other pill colors are the same as dark mode except for that one. Is that on purpose?

@shall0pass
Copy link
Contributor Author

I've been moving most of the purples to purple200. The darker purple, I thought, was harder to see. I lightened the greens on the cleared checkboxes too for the same reason.

@shall0pass
Copy link
Contributor Author

Looking back, that's one of the few were I can change just that property. I think you're right, in that it would match the Due and Missed pills better as a darker shade.

@carkom
Copy link
Contributor

carkom commented Feb 2, 2024

There's a bug where the theme reverts to light if you set theme to Midnight then navigate to the settings page.

@shall0pass
Copy link
Contributor Author

The same bug exists with the development theme. I haven't been able to solve it yet.

@shall0pass
Copy link
Contributor Author

There's a bug where the theme reverts to light if you set theme to Midnight then navigate to the settings page.

I think I found it!

@carkom
Copy link
Contributor

carkom commented Feb 2, 2024

There's a bug where the theme reverts to light if you set theme to Midnight then navigate to the settings page.

I think I found it!

Good find! Is it possible to use the list from theme.tsx for these instead of hard coding them? Would be nice to make this easier for any future theme additions.

@shall0pass
Copy link
Contributor Author

There's a bug where the theme reverts to light if you set theme to Midnight then navigate to the settings page.

I think I found it!

Good find! Is it possible to use the list from theme.tsx for these instead of hard coding them? Would be nice to make this easier for any future theme additions.

I've tried several things, but I'm not sure how to go about it. Everything I've tried has caused the backend to become unresponsive. I could use a pointer. I've tried exporting 'const themes', which didn't go well.

@carkom
Copy link
Contributor

carkom commented Feb 3, 2024

No worries. Probably better as a new PR anyway. I can forsee that the theme list could get crazy long. We'll need a clever way to manage a multitude of themes to avoid that in the future.

@youngcw
Copy link
Member

youngcw commented Feb 3, 2024

I can forsee that the theme list could get crazy long. We'll need a clever way to manage a multitude of themes to avoid that in the future.

I had imagined a way for users to add their own themes via the ui, then the theme logic could see the built in themes and the imported themes. That way we can let people make their own themes and not have to worry about maintaining a bunch of kinda done themes. It would also be cool if we could choose which dark/light themes get used by the system theme option.

@youngcw
Copy link
Member

youngcw commented Feb 3, 2024

Notes:

  • Cashflow report has a hard coded black 0 line. That should be changed probably
  • Custom Report options toggles are missing a border, and look weird when off.
  • Custom report types that are unavailable with current settings are basically the same color as the available ones
  • This is for all themes, but I think that the skull and caution icons for the schedules lines in the account page should have their own color instead of matching the text colors of the pills. That way they wouldn't have to be washed out in dark modes.

@shall0pass
Copy link
Contributor Author

I had imagined a way for users to add their own themes via the ui, then the theme logic could see the built in themes and the imported themes. That way we can let people make their own themes and not have to worry about maintaining a bunch of kinda done themes. It would also be cool if we could choose which dark/light themes get used by the system theme option.

That could possibly be done by adding one more combo box in the settings menu and choosing a "light" theme in one box and a "dark" theme in the other. Then the icon on the budget screen could go back to a simple toggle without a menu. Additionally, while we're talking of future ideas with themes, having the ability to drop a custom file in a folder (maybe server-files) and having the UI pick up on new theme options without having to rebuild would be nice. Though, it's all kind of separate from pulling a list of available themes from one location.

@carkom
Copy link
Contributor

carkom commented Feb 3, 2024

Honestly, I don't see a need for the icon to be on the title bar. Most users don't have a need to change the theme on a regular basis. Most people choose a theme and then never touch that setting again.

I'd be in favor of only having the theme choice in the settings page.

@shall0pass
Copy link
Contributor Author

shall0pass commented Feb 3, 2024

Notes:

  • Cashflow report has a hard coded black 0 line. That should be changed probably
  • Custom Report options toggles are missing a border, and look weird when off.
  • Custom report types that are unavailable with current settings are basically the same color as the available ones
  • This is for all themes, but I think that the skull and caution icons for the schedules lines in the account page should have their own color instead of matching the text colors of the pills. That way they wouldn't have to be washed out in dark modes.

I've addressed the toggles. This is what is included in the latest commit.... the background is lighter than the highlight and background.
image
gray400

I considered this, where the color is between the highlight and background, but didn't really feel like the effect was distinctive enough.
image
gray600

I also addressed the disabled text and icon colors:
image

I haven't found the cashflow line colors yet.... still looking.

4th item: I've lightened the text, which uses the same variable as the icons on the right. I think it helps.
image

@carkom
Copy link
Contributor

carkom commented Feb 3, 2024

Cash flow lines are still hard coded. They don't use the theme colors.

@youngcw
Copy link
Member

youngcw commented Feb 14, 2024

Honestly, I don't see a need for the icon to be on the title bar. Most users don't have a need to change the theme on a regular basis. Most people choose a theme and then never touch that setting again.

I'd be in favor of only having the theme choice in the settings page.

The only reason I would be concerned about doing this is that its nice to switch back and forth quickly to compare colors between themes. Like when making/considering a change being able to not leave the page and compare colors is really handy.

@carkom
Copy link
Contributor

carkom commented Feb 15, 2024

The only reason I would be concerned about doing this is that its nice to switch back and forth quickly to compare colors between themes. Like when making/considering a change being able to not leave the page and compare colors is really handy.

I agree, it definitely makes dev easier. However, we could easily create a dev state that allows this while developing. I don't think we should have features in the app full time just because it makes dev easier, we should solve those dev concerns separately.

@shall0pass shall0pass merged commit 0c5bce8 into actualbudget:master Feb 16, 2024
19 checks passed
@trafico-bot trafico-bot bot added ✨ Merged Pull Request has been merged successfully and removed ✅ Approved labels Feb 16, 2024
@Redbox3070 Redbox3070 mentioned this pull request Feb 24, 2024
1 task
FlorianLang06 pushed a commit to FlorianLang06/actual that referenced this pull request Mar 7, 2024
* midnight updates

* updates

* updates

* background one shade darker

* change highlights to match other accents

* release note

* link color

* variable spelling

* Upcoming pill color

* theme switch bug

* remove development, type error

* toggle background, disabled background+text

* account pillboxes and icons

* typecheck error
@shall0pass shall0pass deleted the midnight branch March 15, 2024 17:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✨ Merged Pull Request has been merged successfully
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants