-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Feature dark mode #2414
Feature dark mode #2414
Conversation
ced78d0
to
d6c971e
Compare
The css resources are not build automatically.
|
Looks like the compass build doesn't work correctly - I've added in the generated CSS files which is a good idea anyhow. The SASS build should probably not be on the build critical path anyway. |
fbad804
to
7dbc99a
Compare
@ripcurlx did you get a chance to fire this up? |
I'm not sure if it is a good idea to put precompiled files in the project, as this might cause confusion if someone changes the css code and later the sass files are processed and overwrite the changes. I think it should work that if someone runs Is there a reason why it is not working like that? |
Works for me now with the pre-compiled css files. As mentioned before I'll give a more in-depth review as soon as the new release is out. Should be mid/end of next weeks. |
Thanks for the feedback. Indeed, it would be good to coordinate with the color code update effort. As part of this CSS refactoring I consolidated the colors into one place, it should be more straightforward to change them: @pedromvpg please let me know if I can help. In the meantime I'll fix the remaining couple issues like the tooltip background so that all of the palette can be controlled from the main configuration SASS file. |
Agreed, that would be the way most devs expect it to work. (Though it can be argued that the SASS files are not likely to change very often, and the Compass build step does add a few seconds to the build. Maybe a separate build step and a "this file is generated from..." warning at the top of the CSS files is a decent compromise?)
I couldn't replicate the issue in a fresh environment, am getting the |
93b87c8
to
0c1d6b6
Compare
@peterzen Just wanted to let you know, that I'm looking into your PR right now. Feedback should be available soon. |
Thanks, please let me know if anything is unclear or wrong :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left a couple of first remarks. In general I like the new structure a lot as it makes it easier to maintain, but I still have issues getting it to run on my local setup. Compass compile is running and I have the main css and themes css in my bisq/desktop/styles dir, but it seems to have issues loading them during startup:
Feb. 28, 2019 5:10:50 NACHM. com.sun.javafx.css.StyleManager loadStylesheetUnPrivileged
WARNUNG: Resource "/bisq/desktop/styles/theme-light.css" not found.
Feb. 28, 2019 5:10:51 NACHM. com.sun.javafx.css.StyleManager loadStylesheetUnPrivileged
WARNUNG: Resource "/bisq/desktop/styles/bisq.css" not found.
No idea why it uses the german locale just for this block log outpout. I'll see if I find the issue regarding the css loading. For you it works out-of-the-box?
build.gradle
Outdated
@@ -281,6 +287,7 @@ configure(project(':desktop')) { | |||
compile project(':p2p') | |||
compile project(':core') | |||
compile project(':common') | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need for the extra new line.
common/src/main/proto/pb.proto
Outdated
@@ -1292,6 +1292,7 @@ message PreferencesPayload { | |||
string rpc_user = 47; | |||
string rpc_pw = 48; | |||
string take_offer_selected_payment_account_id = 49; | |||
bool use_dark_theme = 50; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if we should use a boolean or if we want to have it more future proof if more themes are created over time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's indeed more future proof to use a string so that this file and the settings object won't require changing later.
@@ -25,6 +25,7 @@ | |||
|
|||
public class GlobalSettings { | |||
private static boolean useAnimations = true; | |||
private static boolean useDarkTheme = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason why we need it in the GlobalSettings object?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, this was an oversight on my part -- thanks for catching it.
@@ -0,0 +1,108 @@ | |||
|
|||
@import "_variables.scss"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it necessary to import it here as well, as it is already imported in _base-colors?
237a0aa
to
e4bb301
Compare
@ripcurlx The changes you requested are now done, thanks for the review. Instead of the boolean type light/dark switcher there's now a drop-down menu in the Settings that, as you point out, is more flexible when it comes to additional themes. Please let me know what you think of this solution. |
Those messages seem to indicate that the CSS files were not generated in the I'm getting a clean build when running on a fresh system (Ubuntu 18.04) with
Are you running the build in an existing local copy? Perhaps Maven is caching its dependencies and doesn't pick up the Compass dependency properly - unfortunately I'm not well versed enough with Gradle/Maven but it sounds like the problem might be there somewhere. Could you perhaps try running the build on a clean working copy or maybe a fresh system inside Docker and see if it works that way? Another idea to try would be to move away |
e4bb301
to
bc7c082
Compare
Rebased onto |
@peterzen Sorry again for the delayed response. We just have too many releases recently 😉. I'll try and response in more detail this afternoon. |
Another thing we need to solve is, that there is no need to manually run the compile compass command after a change in the scss within IntelliJ. So developers who are working on the UI don't need to do anything manual that needs to be communicated. |
I'm running it from the CLI most of the time, but sometimes also from within the IntelliJ debugger. |
|
bc7c082
to
12e65ea
Compare
This came out by accident - the white panel that's between the gradient background (which is actually invisible ATM as the white panel is opaque) is transparent. I thought this looked rather cool so left it as transparent but it can of course be colored. I'll fix the other items (buttons, nav etc) I'll wait for all of your comments to come in as it's less work to do them all at once. Are there any other GUI related items currently in the PR pipeline that are worked on currently and may get merged in the coming weeks? If so, it would be good to coordinate them with this PR - possibly even work on them on top of this refactored CSS because in case there are larger changes in the current |
All redesign tasks are on hold until the DAO launch. So now would be a good to get this finished and into master before any further design work will happen. I think it also makes sense to merge this PR first without the dark mode, so there is more time to make it perfect, but still have this structural changes in asap. Unfortunately I won't be able to look at every screen for differences to the current state. Maybe you could do this and I'll have another look at it when you're done. Would this work for you? |
I've been looking at the screens for a little too long to spot small differences so input from a fresh set of eyeballs would be great ;) I'll have a go and go through them again, now after not having worked on them intensively for a couple weeks I'm noticing some differences which I'll fix for sure. |
Yes, you get colorblind after some time 😉. Maybe you could use some image diff tool to help you out like: https://www.diffchecker.com/image-diff. |
aedbbe6
to
085ede7
Compare
085ede7
to
8ed3629
Compare
1b7b591
to
bc0958f
Compare
Rebased onto |
@ripcurlx What is the plan for integrating this PR? There are some new conflicts now due to the DAO changes - I wonder what would be an appropriate time for doing a final rebase to add any other UI changes that might be in the pipeline. |
bc0958f
to
f35a0d5
Compare
f35a0d5
to
c575210
Compare
c575210
to
203bbaa
Compare
^ Rebased onto |
This is really cool, but I was kinda wondering what's the purpose of adding the com.github.anbuck.compass plugin? Does it use some kind of CSS inheritance to build CSS files? Since Bisq holds hot wallet private keys, it's probably best to try to minimize the amount of external code we use, even for development use only. |
The plugin is a bridge to the SASS/Compass interpreter, it basically runs Compass on the SCSS sources. It was introduced in order to break up the monolithic CSS code into smaller, modular chunks and make the color scheme definitions configurable (so that more themes can be added with relative low amount of effort). |
From commit 9bf3016 in PR bisq-network#2414 by @peterzen
f78dfe3
to
84f5a94
Compare
^Rebased on master |
From commit 9bf3016 in PR bisq-network#2414 by @peterzen
Dark mode implementation (#2286 )
(For testing, use
Ctrl+D
to quickly cycle through the light/dark themes)