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

ISPN-15105 Console black theme #409

Closed
wants to merge 2 commits into from

Conversation

himanshu608
Copy link
Collaborator

@himanshu608 himanshu608 commented Oct 8, 2023

Closes: #396

Description

Feature to toggle between Dark/Light theme.

Light Theme:
Screenshot 2023-10-08 184520
Dark Theme:
Screenshot 2023-10-08 184542

How Has This Been Tested?

Tested manually on browser.

Test Impact

Only UI change.

Request review criteria:

Self checklist (all need to be checked):

  • The developer has manually tested the changes and verified that the changes work.
  • Commit message includes a reference to the corresponding JIRA issue (eg. commit message: "ISPN-1234 JIRA TITLE...").
  • Commits have been squashed and self-contained units of work (e.g. 'WIP' and 'Implements feedback' style messages have been removed).
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has added tests or explained why testing cannot be added.
  • Included any necessary screenshots or gifs if it was a UI change.

@dpanshug dpanshug self-requested a review October 9, 2023 06:24
Copy link
Collaborator

@karesti karesti left a comment

Choose a reason for hiding this comment

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

Hi, thanks for the contributions!!
:))

Please include in this PR only the changes of the commit for black theme. Otherwise is too hard for me to review

Concerning black theme:

  1. There are a couple of components that don't apply the thème properly, and so the labels are not displayed:

Health in cluster page
Screenshot 2023-10-12 at 17 02 43

Status in Data container

Screenshot 2023-10-12 at 17 02 35

  1. the fact that we reload the page (F5) makes the user loose the preferences of having the black theme. This is the way users use the console a lot. Would be useful to keep in a local cookie in the browser the preference for the black thème. We don't track connected users, the server is stateless, so we can't store this information somewhere else.

  2. The cache detail tab is not well adapted. Probably something asking for "white" directly in the code

Screenshot 2023-10-12 at 17 06 41

  1. would be nice to see if we can use the highlighter

Screenshot 2023-10-12 at 17 07 55

@karesti karesti removed the request for review from dpanshug October 15, 2023 08:12
@karesti
Copy link
Collaborator

karesti commented Oct 20, 2023

@himanshu608 are you still interested in finishing the task ? thanks! :)

@himanshu608
Copy link
Collaborator Author

@himanshu608 are you still interested in finishing the task ? thanks! :)

I was busy on some other projects. I will continue it now.

The 4th point : would be nice to see if we can use the highlighter.
Please elaborate it.

@karesti
Copy link
Collaborator

karesti commented Oct 23, 2023

@himanshu608 the syntax highlight uses the github theme. we need to adapt it to use a dark theme when displaying entries when dark theme is chosen.

https://github.com/infinispan/infinispan-console/blob/main/src/app/Caches/Entries/CacheEntries.tsx#L125

@himanshu608
Copy link
Collaborator Author

himanshu608 commented Oct 24, 2023

@himanshu608 the syntax highlight uses the github theme. we need to adapt it to use a dark theme when displaying entries when dark theme is chosen.

https://github.com/infinispan/infinispan-console/blob/main/src/app/Caches/Entries/CacheEntries.tsx#L125

Okay got it.., and also.

  1. We were setting style : backgroundColor : 'white' and color : 'white' for some components, I have removed it and it is adapting to the theme now.
  2. For not loosing the preference by F5 refresh, we can store theme cookie in localStorage. I will include that in PR.
  3. I am looking into the syntax highlighter theme, we are using react-syntax-highlighter and githubGist. style,

For SyntaxHighlighter, we can toggle theme by Implementing ThemeProvider and theme state Context,
If we go with toggling theme using context, what should we choose for dark theme instead of githubGist ??

@karesti
Copy link
Collaborator

karesti commented Oct 26, 2023

@himanshu608 I don't have any strong opinon in which is the theme that adapts best since I did not find any theme with the exact grey background as patternfly black theme. Any dark theme should be better than the white gist theme

@himanshu608
Copy link
Collaborator Author

himanshu608 commented Oct 27, 2023

@karesti , I have created a new PR with commits related to these changes only,

  1. In Health cluster Page, we are setting chart_global_label_Fill.value as default, I have checked PatternFly dark theme docs, they did not specify override for this global css variable.
  2. The legend label color is fixed, #151515, That's why it is not adapting to the theme,
    I have tried different ways by using custom labelComponent also searched patternfly docs but no help, Even patternfly official docs website is not adapted for this. you can check out chard section and try toggling the dark theme and check the chard labels.

@karesti karesti closed this Oct 27, 2023
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.

[Feature Request]: Console black theme
2 participants