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

fix: ui dark theme transition #12068

Merged
merged 4 commits into from
May 24, 2023

Conversation

saumeya
Copy link
Contributor

@saumeya saumeya commented Jan 21, 2023

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

closes #11938

closes #11087

also fixes diff checkboxes "Compact diff" and "Inline diff" are white in white
image

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).

@codecov
Copy link

codecov bot commented Jan 21, 2023

Codecov Report

Patch coverage has no change and project coverage change: -0.15 ⚠️

Comparison is base (67c254e) 49.17% compared to head (6ed04da) 49.03%.

❗ Current head 6ed04da differs from pull request most recent head 2466168. Consider uploading reports for the commit 2466168 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #12068      +/-   ##
==========================================
- Coverage   49.17%   49.03%   -0.15%     
==========================================
  Files         248      246       -2     
  Lines       42872    42519     -353     
==========================================
- Hits        21084    20849     -235     
+ Misses      19687    19558     -129     
- Partials     2101     2112      +11     

see 27 files with indirect coverage changes

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

@include themify($themes) {
color: themed('text-2');
}
color: $argo-color-gray-8;
Copy link
Contributor

@keithchong keithchong Feb 21, 2023

Choose a reason for hiding this comment

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

Question: does this apply for light mode only? (Or, maybe this should not be part of the fix?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it is not related to transition, just a small fix related to compact diff menu
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does relate to dark/light mode change

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, the text was not showing.

@saumeya saumeya force-pushed the ui-transition-dark-mode branch from 4c1cb75 to dec7af5 Compare March 22, 2023 11:36
@saumeya saumeya requested a review from keithchong March 22, 2023 12:04
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 I pulled your code and tested locally. I still see the issue of #11938 from time to time when I switch from Settings page to Applications page. Could you confirm if it's fixed for you?

@saumeya
Copy link
Contributor Author

saumeya commented Mar 23, 2023

Works for me

www_screencapture_com_2023-3-23_12_34.mp4

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.

I confirmed again and it looks fine to me. Just left a small comment.

const getBGColor = (theme: string): string => {
return theme === 'light' ? '#dee6eb' : '#100f0f';
};

Copy link
Contributor

Choose a reason for hiding this comment

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

This could be simplified to one line. Also can we use existing colors for the new two colors? For example, ARGO_GRAY4_COLOR is very similar to #dee6eb. Better we don't add more colors to the existing color library.

Suggested change
const getBGColor = (theme: string): string => theme === 'light' ? '#dee6eb' : '#100f0f';

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Solved, thanks Yi
I have used the color codes from the existing color library - can't use the name in .tsx file as argo-ui can't be imported.

Copy link
Contributor

@keithchong keithchong left a comment

Choose a reason for hiding this comment

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

LGTM, excluding the things that Yi mentioned.

@saumeya saumeya requested a review from ciiay March 27, 2023 07:13
@rbreeze rbreeze enabled auto-merge (squash) March 30, 2023 15:27
@keithchong
Copy link
Contributor

/retest

saumeya added 4 commits May 24, 2023 15:24
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>
auto-merge was automatically disabled May 24, 2023 09:57

Head branch was pushed to by a user without write access

@saumeya saumeya force-pushed the ui-transition-dark-mode branch from 6ed04da to 2466168 Compare May 24, 2023 09:57
@saumeya saumeya added the cherry-pick/2.7 Candidate for cherry picking into the 2.7 release branch label May 24, 2023
@keithchong keithchong merged commit 0a8a71e into argoproj:master May 24, 2023
export const Layout = (props: LayoutProps) => (
<div className={props.pref.theme ? 'theme-' + props.pref.theme : 'theme-light'}>
<div className={`cd-layout ${props.isExtension ? 'cd-layout--extension' : ''}`}>
<Sidebar onVersionClick={props.onVersionClick} navItems={props.navItems} pref={props.pref} />
{props.pref.theme ? (document.body.style.background = getBGColor(props.pref.theme)) : null}
Copy link

Choose a reason for hiding this comment

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

@saumeya But this will output the color to the body and break the layout.

image

image

n9 added a commit to n9/argo-cd that referenced this pull request May 30, 2023
This was referenced May 30, 2023
yyzxw pushed a commit to yyzxw/argo-cd that referenced this pull request Aug 9, 2023
* fix: ui dark theme transition

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

* fix checkbox label

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

* review comments

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

* fix lint

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

---------

Signed-off-by: saumeya <saumeyakatyal@gmail.com>
n9 added a commit to n9/argo-cd that referenced this pull request Aug 17, 2023
n9 added a commit to n9/argo-cd that referenced this pull request Aug 17, 2023
n9 added a commit to n9/argo-cd that referenced this pull request Sep 30, 2023
n9 added a commit to n9/argo-cd that referenced this pull request Oct 2, 2023
n9 added a commit to n9/argo-cd that referenced this pull request Oct 2, 2023
n9 added a commit to n9/argo-cd that referenced this pull request Oct 19, 2023
tesla59 pushed a commit to tesla59/argo-cd that referenced this pull request Dec 16, 2023
* fix: ui dark theme transition

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

* fix checkbox label

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

* review comments

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

* fix lint

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
cherry-pick/2.7 Candidate for cherry picking into the 2.7 release branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Transition between pages in dark mode flashes white Dark mode still leaves a white bar at the bottom
5 participants