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 #378

Merged
merged 32 commits into from
Jul 31, 2020
Merged

Dark mode #378

merged 32 commits into from
Jul 31, 2020

Conversation

pedromvpg
Copy link
Member

Add darkmode functionality and new client screen images.

@cbeams
Copy link
Member

cbeams commented Jul 24, 2020

This looks great, wow. A few notes:

  1. Consider defaulting to dark mode when the OS is in dark mode; see https://stackoverflow.com/a/52986538 for the CSS.
  2. Swap the homepage screenshot to Bisq in dark mode when website is in dark mode
  3. Text in the input field for the Markets page is hard to see note how the 'eur' text is dark-on-dark below:

image

@cbeams
Copy link
Member

cbeams commented Jul 27, 2020

Defaulting to dark mode seems to work well (I'm looking at https://deploy-preview-378--bisq-website.netlify.app/). One nit is that when dark mode is active, the menu toggle reads 'Dark', and when light mode is active, it reads 'Light'. Shouldn't this be the other way around? i.e. an affordance should advertise the state change that will occur if you make use of it (click it), as opposed to advertising the current state. Also, a common idiom I've seen is the use of a crescent moon symbol in the upper right for toggling dark / light mode. It takes up less real estate and is somehow more intuitive (to me) than just a textual 'light/dark' link.

@m52go
Copy link
Contributor

m52go commented Jul 27, 2020

Just want to say I've been watching this PR -- it looks amazing!

I see the markets dropdown text has been fixed, but the screenshots on the front page are still in light mode. Also I have agree with cbeams' comment above about flipping the dark mode toggle action and considering symbols instead of text for the label.

Copy link
Contributor

@m52go m52go left a comment

Choose a reason for hiding this comment

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

I had another look and everything seems to work fine. It looks so good (!).

I've attached a couple of nitpicks in the JavaScript...there's a stray console.out() still there from debugging and an unused condition in 2 of the if statements.

That's it from my end. @pedromvpg I will ACK once you make those changes, and merge a short while afterward if there's no other feedback.

_layouts/default.html Outdated Show resolved Hide resolved
_layouts/default.html Outdated Show resolved Hide resolved
_layouts/default.html Outdated Show resolved Hide resolved
pedromvpg and others added 3 commits July 30, 2020 19:41
Co-authored-by: m52go <mfiver@gmail.com>
Co-authored-by: m52go <mfiver@gmail.com>
Co-authored-by: m52go <mfiver@gmail.com>
@pedromvpg pedromvpg requested a review from m52go July 31, 2020 00:04
Copy link
Contributor

@m52go m52go left a comment

Choose a reason for hiding this comment

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

ACK

@m52go m52go merged commit 307383b into bisq-network:master Jul 31, 2020
@pedromvpg pedromvpg deleted the dark-mode branch August 27, 2020 05:00
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.

3 participants