Skip to content

feat: add dark theme to UI#9703

Merged
crenshaw-dev merged 10 commits intoargoproj:masterfrom
saumeya:add-dark-theme-ui
Sep 17, 2022
Merged

feat: add dark theme to UI#9703
crenshaw-dev merged 10 commits intoargoproj:masterfrom
saumeya:add-dark-theme-ui

Conversation

@saumeya
Copy link
Contributor

@saumeya saumeya commented Jun 17, 2022

closes #4722

Dependent on argo-ui PR - argoproj/argo-ui#245

Demo video

online-screen-recorder-2022-06-17--10-32-11.mp4
  • There are a few components like dropdowns, popups and autocomplete that are still not themed. They render differently and can use a separate PR and theme variable would be needed to passed as a prop.

should close #4722

Initial Description that is no longer valid
Steps to build and run, checkout to argo-ui PR branch in your local.
Run yalc publish
Then in this argocd branch cd ui
Run yalc add argo-ui
cd ..
make start

  • Lint is failing because I'm importing a package from argo-ui that is still not merged.

Note on DCO:

If the DCO action in the integration test fails, one or more of your commits are not signed off. Please click on the Details link next to the DCO action for instructions on how to resolve this.

Checklist:

  • Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this does not need to be in the release notes.
  • The title of the PR states what changed and the related issues number (used for the release note).
  • I've included "Closes [ISSUE #]" or "Fixes [ISSUE #]" in the description to automatically close the associated issue.
  • I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.
  • Does this PR require documentation updates?
  • I've updated documentation as required by this PR.
  • Optional. My organization is added to USERS.md.
  • I have signed off all my commits as required by DCO
  • I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
  • My build is green (troubleshooting builds).

@saumeya saumeya force-pushed the add-dark-theme-ui branch from 81b270e to 29b9d1e Compare June 26, 2022 20:30
@saumeya saumeya marked this pull request as ready for review June 27, 2022 06:35
@reginaelyse reginaelyse self-assigned this Jun 28, 2022
Copy link
Contributor

@reginaelyse reginaelyse left a comment

Choose a reason for hiding this comment

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

besides the things you mentioned already in your PR description (dropdowns, popups and autocomplete) that can go into another PR, looks awesome!! Thanks for your hard work on this!

Copy link
Contributor

@ciiay ciiay left a comment

Choose a reason for hiding this comment

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

Hi Saumeya, great work! I played with the dark theme and found a few titles are a little hard to read in dark theme. Other than that all looks great 👍
Screen Shot 2022-06-29 at 5 53 42 PM
Screen Shot 2022-06-29 at 5 54 12 PM
Screen Shot 2022-06-29 at 5 55 24 PM

@saumeya
Copy link
Contributor Author

saumeya commented Jun 30, 2022

Thanks a lot for the review Yi, will definitely correct these styles

@crenshaw-dev
Copy link
Member

@saumeya do you have time to take a look at the CI failures? Looks like probably some relatively simple linting issues.

@saumeya
Copy link
Contributor Author

saumeya commented Aug 16, 2022

@saumeya do you have time to take a look at the CI failures? Looks like probably some relatively simple linting issues.

@crenshaw-dev , The argo-ui PR argoproj/argo-ui#245 needs to be merged first. There is a function I am using from that PR, that is why lint is failing.
Once that is merged, I'll update this PR to take argo-ui latest commit sha and then this won't fail.

@crenshaw-dev
Copy link
Member

@saumeya sweet! I pinged Remington for a review. :-)

@rbreeze
Copy link
Member

rbreeze commented Sep 6, 2022

@saumeya Argo UI PR is approved, can you update this PR with new commit SHA?

@saumeya
Copy link
Contributor Author

saumeya commented Sep 7, 2022

Thanks @rbreeze, could you merge that PR?

@crenshaw-dev
Copy link
Member

@saumeya merged!

@crenshaw-dev
Copy link
Member

Do we need to bump the moment version in argo-ui to get past the Snyk check?

@saumeya
Copy link
Contributor Author

saumeya commented Sep 7, 2022

@crenshaw-dev @rbreeze Updated the PR, now it can be tested directly.

@codecov
Copy link

codecov bot commented Sep 7, 2022

Codecov Report

Base: 45.76% // Head: 45.76% // No change to project coverage 👍

Coverage data is based on head (4dc4914) compared to base (e53ab49).
Patch has no changes to coverable lines.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #9703   +/-   ##
=======================================
  Coverage   45.76%   45.76%           
=======================================
  Files         236      236           
  Lines       28519    28519           
=======================================
  Hits        13053    13053           
  Misses      13661    13661           
  Partials     1805     1805           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@saumeya
Copy link
Contributor Author

saumeya commented Sep 7, 2022

Do we need to bump the moment version in argo-ui to get past the Snyk check?

I can't see the snyk check, need some permission. But it was something in the rebase. Let me update the dependency

@crenshaw-dev
Copy link
Member

@saumeya Snyk is passing now, thanks!

@crenshaw-dev crenshaw-dev added this to the v2.5 milestone Sep 7, 2022
@crenshaw-dev
Copy link
Member

@saumeya just tested locally, and it looks awesome!

Lighthouse is reporting some remaining contrast issues. I think the only ones I'd want to block merging on are these:

image

This is on the Projects page. The top-right header, the logout button, and the tab headings are all very low-contrast and a bit difficult to read.

@saumeya
Copy link
Contributor Author

saumeya commented Sep 12, 2022

Thanks @crenshaw-dev, Will fix these issues!

@saumeya
Copy link
Contributor Author

saumeya commented Sep 13, 2022

@crenshaw-dev Could you approve and merge this PR - argoproj/argo-ui#272

Signed-off-by: saumeya <saumeyakatyal@gmail.com>
Signed-off-by: saumeya <saumeyakatyal@gmail.com>
Signed-off-by: saumeya <saumeyakatyal@gmail.com>
Signed-off-by: saumeya <saumeyakatyal@gmail.com>
Signed-off-by: saumeya <saumeyakatyal@gmail.com>
Signed-off-by: saumeya <saumeyakatyal@gmail.com>
Signed-off-by: saumeya <saumeyakatyal@gmail.com>
Signed-off-by: saumeya <saumeyakatyal@gmail.com>
Signed-off-by: saumeya <saumeyakatyal@gmail.com>

update moment yarn.lock

Signed-off-by: saumeya <saumeyakatyal@gmail.com>

update moment yarn.lock

Signed-off-by: saumeya <saumeyakatyal@gmail.com>
Signed-off-by: saumeya <saumeyakatyal@gmail.com>
@crenshaw-dev
Copy link
Member

Screen.Recording.2022-09-15.at.8.37.00.AM.mov

The contrast looks great now! I really really hate to add one more request, but I think this one is important - the transition between pages flashes white, which is really hard on the eyes. I'm hoping it's a relatively easy fix.

@crenshaw-dev
Copy link
Member

Testing locally, looks like setting the body element's background color to something dark fixes it. I'm not sure if that's the appropriate fix, but it might be an option.

@crenshaw-dev
Copy link
Member

@saumeya I'm going to go ahead and merge this, and we'll work out the background color and any other issues in follow-up PRs. :-)

@crenshaw-dev crenshaw-dev merged commit da9063b into argoproj:master Sep 17, 2022
@saumeya
Copy link
Contributor Author

saumeya commented Sep 18, 2022

Thanks for merging @crenshaw-dev!! We need to imporve as many times possible, i'll look into the transition issue. Also I see your screen height is bigger and so the end of the background is also white. I'll take a look and create fixes.

ashutosh16 pushed a commit to ashutosh16/argo-cd that referenced this pull request Oct 7, 2022
* feat: enable dark theme in ui

Signed-off-by: saumeya <saumeyakatyal@gmail.com>

* add more theme styles

Signed-off-by: saumeya <saumeyakatyal@gmail.com>

* remove package lock

Signed-off-by: saumeya <saumeyakatyal@gmail.com>

* improvements

Signed-off-by: saumeya <saumeyakatyal@gmail.com>

* improvements

Signed-off-by: saumeya <saumeyakatyal@gmail.com>

* cleanup

Signed-off-by: saumeya <saumeyakatyal@gmail.com>

* fix review comments

Signed-off-by: saumeya <saumeyakatyal@gmail.com>

* update argo-ui sha

Signed-off-by: saumeya <saumeyakatyal@gmail.com>

* update moment yarn.lock

Signed-off-by: saumeya <saumeyakatyal@gmail.com>

update moment yarn.lock

Signed-off-by: saumeya <saumeyakatyal@gmail.com>

update moment yarn.lock

Signed-off-by: saumeya <saumeyakatyal@gmail.com>

* fix review comments

Signed-off-by: saumeya <saumeyakatyal@gmail.com>

Signed-off-by: saumeya <saumeyakatyal@gmail.com>
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.

Add dark mode

5 participants