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

Dark Mode #3152

Merged
merged 4 commits into from Aug 29, 2019

Conversation

@wiz
Copy link
Member

commented Aug 27, 2019

Implements basic Dark Mode functionality using the refactored CSS light/dark themes contributed in peterzen/bisq@9bf3016, but without adding new jar deps and without breaking the current light mode theme. Resolves #2286 and closes #2414.

The refactored CSS themes could use further testing/polishing/refactoring by a designer (cc @pedromvpg), but this PR should be good enough to merge for the upcoming v1.1.6 release - any modified CSS color codes can be submitted in a separate PR later.

Screen Shot 2019-08-27 at 23 57 02

Screen Shot 2019-08-28 at 0 06 43

Screen Shot 2019-08-27 at 23 57 11

Screen Shot 2019-08-28 at 0 06 09

Screen Shot 2019-08-27 at 23 53 11

Screen Shot 2019-08-27 at 23 52 52

Screen Shot 2019-08-28 at 2 20 08

Screen Shot 2019-08-28 at 0 06 26

@wiz wiz requested review from ripcurlx and sqrrm as code owners Aug 27, 2019
@ripcurlx

This comment has been minimized.

Copy link
Member

commented Aug 28, 2019

@wiz Thanks for taking over the dark mode handling. @peterzen waited very patiently already to get his changes into Bisq 😄 . I do remember that a couple of color codes of the current version (light_version) weren't right, so someone needs to go through the app and have a look if everything looks as it did before, when light mode is selected.

@wiz

This comment has been minimized.

Copy link
Member Author

commented Aug 28, 2019

I've gone through it quickly and already fixed the major issues I found, but as I mentioned above, I think the CSS code I took from peter's theme needs to be tested and further polished, such as the shades of gray being slighly too light or too dark gray, or maybe some 1px border or other minor things, but IMO those are trivial enough to be fixed as bugs in separate PRs by @peterzen later. I'd like to see this get merged in time for the v1.1.6 release if possible 😅

@ripcurlx

This comment has been minimized.

Copy link
Member

commented Aug 28, 2019

I've gone through it quickly and already fixed the major issues I found, but as I mentioned above, I think the CSS code I took from peter's theme needs to be tested and further polished, such as the shades of gray being slighly too light or too dark gray, or maybe some 1px border or other minor things, but IMO those are trivial enough to be fixed as bugs in separate PRs by @peterzen later. I'd like to see this get merged in time for the v1.1.6 release if possible 😅

As there is still time until Friday it would be great to get it as clean as possible, so it is not a downgrade for everyone not using the dark theme 😉

@ripcurlx

This comment has been minimized.

Copy link
Member

commented Aug 28, 2019

Maybe @pedromvpg could give some feedback on the dark mode and things that he recognizes that are not correct for the light mode atm.

@wiz

This comment has been minimized.

Copy link
Member Author

commented Aug 28, 2019

Sorry, I'm a bit confused. The intended scope of this PR is merely to implement Dark Mode in a way that doesn't add jar deps. I took the CSS code from @peterzen and already fixed the 12 or so major UI issues that were immediately visible to me. @ripcurlx could you clarify what action we are currently waiting from, and by whom?

Actually, could you please help to clarify my understanding and expectations about how the entire PR workflow and release process in Bisq DAO should work? Recently I am concerned about the "PR Limbo" problem - but first let me verify my current understanding:

  1. This PR is still actively and currently assigned to @ripcurlx and @sqrrm for review, not @pedromvpg or anyone else. Since it seems neither reviewer has reviewed or tested the PR yet, the action we are currently waiting from is a review with comments from one of them. If the reviewer(s) wish to assign review to another person, they would do so with the "assign" feature in GitHub.

  2. If @peterzen or @pedromvpg want to tweak the light/dark themes or other CSS code further, they are free to do so in their own separate PR(s) after this PR is merged, but they are not currently members of this PR and it is still only pending action from either reviewer.

  3. I expect the reviewer(s) of the PR to actually review and test this PR by themselves, in addition to any outside reviewers assigned. If anyone finds issues with this PR, they should report them here and I will amend the PR if necessary. If there are no issues from the reviewers, they should proceed with merging.

  4. When Devin and I do the full release testing this weekend for v1.1.6, if we find any major UI issues caused by this PR then we should either fix them at that time (again, as part of completely separate bugfix PRs) or we should revert this PR entirely to prevent those UI issues from making it into the v1.1.6 release.

If the above is correct, then can you please tell me why you haven't taken any action on this PR yet? Please understand that my intention is not to criticize any person or process, it's mainly to avoid PR limbo, after seeing some issues open for literally years on here I want to clarify who is actively assigned, and what the action is to take. Maybe we should make a flowchart for this workflow process?

@mrosseel

This comment has been minimized.

Copy link
Member

commented Aug 28, 2019

Just FYI:
In the contributor checklist (https://docs.bisq.network/contributor-checklist.html) it's mentioned the C4 process is used (also used by zeromq and monero).

So either C4 is not perfect or we're not perfectly following it :)

@ripcurlx

This comment has been minimized.

Copy link
Member

commented Aug 28, 2019

@wiz No offense taken 😄.
The reason for my remarks are that I do remember when I tested @peterzen PR the last time that there were still some color codes/styles wrong in the light mode. Maybe you fixed them all already. As time is precious I just want to prevent that when I test this PR that I'll see immediately a couple of style issues on the first screen already. But if you think that the current state for the light theme looks good to you already I'm happy to review the PR.

@wiz

This comment has been minimized.

Copy link
Member Author

commented Aug 28, 2019

Of course I respect your time; I would not submit a PR for your review if I did not feel it was ready for your review. And maybe you will find bugs on the first screen, which is why we have the review process in the first place. But as I previously stated, I already fixed about 12 of the UI issues I found in the imported CSS theme from @peterzen, and while there might be some pixel-level or gray-shading-level issues remaining, I feel these bug fixes should be submitted in a different PR and are not significant enough to prevent merging this PR as-is, but of course as the reviewer that is your decision to make, and as you can only make the decision to approve, reject, or request changes to this PR by actually reviewing and testing the PR first, yes, please review this PR as you are the currently assigned person to work on it 😅

Again this is not a personal criticism but something I want to try and improve in Bisq DAO workflow. As a contributor, I only ask that you try to strictly follow the PR workflow to avoid PRs falling into limbo like they have in the past. For example, if you want Pedro to review the code that's your decision, but in that case please assign him as the reviewer, and unassign yourself. Otherwise you'd still be assigned and responsible for working on the PR but not actually working on it, and since Pedro isn't assigned he isn't responsible for it, so you'd be neglecting your responsibility as the currently assigned reviewer.

@peterzen

This comment has been minimized.

Copy link
Contributor

commented Aug 28, 2019

@wiz Thanks for taking over the dark mode handling. @peterzen waited very patiently already to get his changes into Bisq . I do remember that a couple of color codes of the current version (light_version) weren't right, so someone needs to go through the app and have a look if everything looks as it did before, when light mode is selected.

I'm happy to see this moving towards being merged.

However, I have to say I'm a bit puzzled by the process - my PR was sitting idle for several months waiting on designer input to get a couple color codes straightened out and any comments/feedback from other developers, which I was of course happily going to implement. Instead of that happening, someone else creates another PR based on mine in a matter of hours and makes the changes directly.

No hard feelings and again, I'm happy that this is making its way into the release but this process does feel a little sour and unheard of in the open source projects I've contributed to so far. (It's my first contribution to Bisq so of course it could just be my ignorance that I wasn't aware of some aspect of the PR review workflow, in which case I apologise for not being aware of it.)

@wiz @ripcurlx care to comment?

@wiz

This comment has been minimized.

Copy link
Member Author

commented Aug 28, 2019

@peterzen Yeah... I've only recently started contributing to Bisq myself, but the overall lack of a proper PR workflow and many issues going into "PR limbo" for months or years is something I hope to improve now that I'm here... Obviously it's not cool that Bisq left your PR open for so long and I want to raise the bar going forward.

Sorry to be the bearer of bad news but your PR was not approved because it added too many external jar dependencies, and also had a bunch of weird unrelated stuff in the PR. I took the liberty of rewriting the code in a simpler way, while keeping your refactored CSS theme with you set as the Author on that commit so you still get credit for your contributions, and after this PR gets merged then you can finally proceed to polish the Dark Mode as you see fit.

I hope this doesn't discourage you from making future PRs, and I invite you to join me on Slack to discuss how we can further improve the Light Mode and Dark Mode UI together. Look on the bright side, now you will finally be a Bisq contributor 😀

@battleofwizards

This comment has been minimized.

Copy link
Contributor

commented Aug 28, 2019

@peterzen I am just a spectator who joined the project recently (not involved in this PR). The way I see it is that Bisq is very short on experienced reviewers and technical project management.

For that reason it often requires some pushing and management by the PR author to get the changes merged in.

In any case, you definitely should be credited for your hard work on this regardless of who does the "final push".

@peterzen

This comment has been minimized.

Copy link
Contributor

commented Aug 28, 2019

@peterzen Yeah... I've only recently started contributing to Bisq myself, but the overall lack of a proper PR workflow and many issues going into "PR limbo" for months or years is something I hope to improve now that I'm here... Obviously it's not cool that Bisq left your PR open for so long and I want to raise the bar going forward.

The PR was not sitting there for no reason, there have been other priorities in the previous months. This does happen and it's not a problem as far as I'm concerned.

Sorry to be the bearer of bad news but your PR was not approved because it added too many external jar dependencies, and also had a bunch of weird unrelated stuff in the PR. I took the liberty of rewriting the code in a simpler way, while keeping your refactored CSS theme with you set as the Author on that commit so you still get credit for your contributions, and after this PR gets merged then you can finally proceed to polish the Dark Mode as you see fit.

Dude, there was 1 additional jar dependency, the Compass plugin, which is a compile time dependency so isn't even part of the code of the app. Also, in a usual PR process you could have pointed out the "weird unrelated stuff" (I have no idea what you're referring to but would love to know), @ripcurlx didn't point out any such thing in his code review.

It would have been much more constructive if you provided your input which I would have considered. In fact you and I had an exchange about this but you resolved it by arbitrarily copying the original PR and changing it the was you saw fit, without caring about consensus.

I hope this doesn't discourage you from making future PRs, and I invite you to join me on Slack to discuss how we can further improve the Light Mode and Dark Mode UI together. Look on the bright side, now you will finally be a Bisq contributor

As a matter of fact, it does discourage me and it should any other contributor.

@peterzen

This comment has been minimized.

Copy link
Contributor

commented Aug 28, 2019

@peterzen I am just a spectator who joined the project recently (not involved in this PR). The way I see it is that Bisq is very short on experienced reviewers and technical project management.

For that reason it often requires some pushing and management by the PR author to get the changes merged in.

In any case, you definitely should be credited for your hard work on this regardless of who does the "final push".

OK fair enough, as I said I'm happy that this is moving forward - but I still think it would have been much nicer if @wiz added a couple of lines of comment which I would have turned around in a day. That way, the contributor process would have been closely followed - in this scenario how could I possibly sign my patch (in accordance with the C4 process), it just doesn't feel right.

I don't mean to make a fuss of this and don't want to waste any more developer time - it's just that if a contributor did this to another contributor's patch in, say, the Decred project to which I contribute a lot, he/she would have been frowned upon to say the least.

@ripcurlx

This comment has been minimized.

Copy link
Member

commented Aug 28, 2019

@peterzen I take the blame on having this PR around for such a long time. But that is not for no reason. As we had and partly still have quite limited dev resources to review existing PRs I just didn't have the time back then to look and finish off properly the existing PR. As far as I still remember what was left to ACK it were:

  • exiting style issues in the light mode
  • build setup didn't work for me out-of-the-box on Intellij

The big difference to this PR is, that yours contains also css clean-up and modularization of the existing CSS codebase, which was a wish from my side, as the current css was getting harder and harder to maintain.

@wiz Regarding the compass CSS dependency, as it only runs on compile time I don't think it is a big security risk as any code changes outside of the CSS file would pop up in the commit anyways.

Not to consume more time of everyone I see following solution. Using this PR and incorporate everything which is missing from @peterzen's PR (@wiz I'll mention the required changes in the review) or breaking up @peterzen's PR in one with new color schemes (adding the css fixes from @wiz) and another one containing the css code modularization part until the development build works and a consensus is found there regarding the additional dependency (there are obviously different positions existing in the contributor base). I'm fine with either solution - I just want to get things done and it would be great to have the dark style option integrated in Bisq.

What do you guys think?

Copy link
Member

left a comment

NACK - Please have a look at my comment on the data type of the UI theme property. I'll have a look at the css color codes next.

common/src/main/proto/pb.proto Outdated Show resolved Hide resolved
core/src/main/java/bisq/core/user/Preferences.java Outdated Show resolved Hide resolved
@peterzen

This comment has been minimized.

Copy link
Contributor

commented Aug 28, 2019

@peterzen I take the blame on having this PR around for such a long time. But that is not for no reason. As we had and partly still have quite limited dev resources to review existing PRs I just didn't have the time back then to look and finish off properly the existing PR.

I meant to say I know there were good reasons this PR wasn't prioritized over the DAO work going on at the time, apologies for my lousy double negative wording :)

Not to consume more time of everyone I see following solution. Using this PR and incorporate everything which is missing from @peterzen's PR (@wiz I'll mention the required changes in the review) or breaking up @peterzen's PR in one with new color schemes (adding the css fixes from @wiz) and another one containing the css code modularization part until the development build works and a consensus is found there regarding the additional dependency (there are obviously different positions existing in the contributor base). I'm fine with either solution - I just want to get things done and it would be great to have the dark style option integrated in Bisq.

What do you guys think?

I'm happy with this suggestion. The only thing is, the dark color scheme in #2414 is dependent on the modularized CSS, it would be very time consuming to extract the colors into style.css in its current form.

I'm looking into how to best get the color scheme fixes from #3152 into #2414.

@peterzen

This comment has been minimized.

Copy link
Contributor

commented Aug 28, 2019

This PR imported code from a very outdated version of #2414, which doesn't yet have the changes requested by @ripcurlx , i.e. the theme selector is no longer a bool and the GlobalSettings object is out.

@peterzen

This comment has been minimized.

Copy link
Contributor

commented Aug 28, 2019

@wiz would you be willing to merge your color scheme fixes into https://github.com/peterzen/bisq/tree/feature-dark-mode/desktop/src/main/resources/styles ?

@wiz

This comment has been minimized.

Copy link
Member Author

commented Aug 28, 2019

@peterzen You don't need to ask my permission, everything I contribute to Bisq is licensed under the GNU AGPL: https://github.com/bisq-network/bisq/blob/master/LICENSE

@peterzen

This comment has been minimized.

Copy link
Contributor

commented Aug 28, 2019

@peterzen You don't need to ask my permission, everything I contribute to Bisq is licensed under the GNU AGPL: https://github.com/bisq-network/bisq/blob/master/LICENSE

I wasn't asking for your permission, was just wondering if you would be willing to help out with this.

@wiz

This comment has been minimized.

Copy link
Member Author

commented Aug 28, 2019

This PR does help you out, it will merge your CSS refactoring together with my fixes for your refactoring into the Bisq master branch, and then you can merge master into your PR's work-branch and request another review of your PR for the additional features and functionality you want to add.

@peterzen

This comment has been minimized.

Copy link
Contributor

commented Aug 28, 2019

This PR does help you out, it will merge your CSS refactoring together with my fixes for your refactoring into the Bisq master branch, and then you can merge master into your PR's work-branch and request another review of your PR for the additional features and functionality you want to add.

The code this PR is based on is outdated and buggy.

@wiz wiz force-pushed the wiz:darkmode branch from 50a8f26 to 61976b0 Aug 29, 2019
wiz and others added 3 commits Aug 26, 2019
* Store preference for Dark Mode as int32 css_theme
Co-authored-by: Peter Banik <peter@froggle.org>
@wiz wiz force-pushed the wiz:darkmode branch from 61976b0 to fd0c865 Aug 29, 2019
@wiz

This comment has been minimized.

Copy link
Member Author

commented Aug 29, 2019

@ripcurlx the requested changes have been made, ready for re-review

Copy link
Member

left a comment

ACK - Thanks @wiz and @peterzen for making this happen! This feature will be part of v1.1.6.

@ripcurlx ripcurlx merged commit 3ccae69 into bisq-network:master Aug 29, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@wiz wiz deleted the wiz:darkmode branch Aug 29, 2019
@pedromvpg

This comment has been minimized.

Copy link
Member

commented Sep 5, 2019

Very cool!
I would only suggest to maybe have the active state of the main nav button the dark green (#00652D) the the type the light green (#00B60F) instead of the dark grey and the white. This would make the navigation more unified.

Bisq - Main Nav Active Button State

@ripcurlx ripcurlx referenced this pull request Sep 6, 2019
@wiz wiz referenced this pull request Sep 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.