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 #4257 fixed ui for mermaid diagrams in dark mode #4289

Merged
merged 6 commits into from
Jan 13, 2025

Conversation

X1Vi
Copy link
Contributor

@X1Vi X1Vi commented Jan 11, 2025

fixes #4257
The diagrams are now visible and can be seen easily in dark mode

image

@X1Vi X1Vi requested a review from boojack as a code owner January 11, 2025 04:54
@RoccoSmit
Copy link
Contributor

@X1Vi , by forcing dark mode light mode now has the issue dark mode had. Are you able to set the values dynamically based on the user settings?

Current light mode:
image

PR light mode:
image

@X1Vi
Copy link
Contributor Author

X1Vi commented Jan 11, 2025

Fixed the issue
I am dynamically changing the values now.
Thanks for the advice.

When Dark theme
image

When light theme
image

@RoccoSmit
Copy link
Contributor

RoccoSmit commented Jan 11, 2025

When you select Follow System as the theme the commonContext.appearance value is system, meaning that system set dark mode will use light mode styles.

Have a look here to see how this has been handled in the past

@X1Vi
Copy link
Contributor Author

X1Vi commented Jan 11, 2025

When you select Follow System as the theme the commonContext.appearance value is system, meaning that system set dark mode will use light mode styles.

Have a look here to see how this has been handled in the past

Can you elaborate more as to what you want exactly ? because if it is system it just goes to default.

@RoccoSmit
Copy link
Contributor

RoccoSmit commented Jan 11, 2025

@X1Vi , there are 3 theme options

Light -> Always use light theme,
Dark -> Always use dark theme,
Follow System -> Use light or dark based on what your OS/browser is set to use

When you select Follow System and your OS/browser is set to dark mode, your code will see system != dark and show the diagram in light mode while the rest of the app loads in dark mode

My suggestion was to either

  • use the code linked above that uses getSystemColorScheme in utils.ts to get the light or dark value from system
  • use the local storage value that gets set in the above linked code

@johnnyjoygh added a 3rd option of using the useColorScheme method from the @mui/joy library to get the theme to use

@X1Vi
Copy link
Contributor Author

X1Vi commented Jan 11, 2025

@johnnyjoygh I have made a new and better solution I wasn't completely aware that how the theme works completely (Didn't knew there was another hook as well). I have selected themes which I sought suitable if you feel like they are not suitable then feel free to ping me. I think we will have to just adjust the themes.

@X1Vi X1Vi requested a review from johnnyjoygh January 12, 2025 04:52
@X1Vi
Copy link
Contributor Author

X1Vi commented Jan 12, 2025

I have fixed the changes as told by @RoccoSmit

Copy link
Contributor

@RoccoSmit RoccoSmit left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines 11 to 14

const handleMermaidTheme = () => {
return mode == "dark" ? "dark" : "default";
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
const handleMermaidTheme = () => {
return mode == "dark" ? "dark" : "default";
};
const mermaidTheme = mode == "dark" ? "dark" : "default";

Copy link
Collaborator

@johnnyjoygh johnnyjoygh left a comment

Choose a reason for hiding this comment

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

Please use const instead function call.

@X1Vi
Copy link
Contributor Author

X1Vi commented Jan 13, 2025

Please use const instead function call.

Done

Copy link
Collaborator

@johnnyjoygh johnnyjoygh left a comment

Choose a reason for hiding this comment

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

LGTM

@johnnyjoygh johnnyjoygh merged commit 9451749 into usememos:main Jan 13, 2025
2 checks passed
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.

Mermaid Diagrams Are Unreadble in Dark Theme
3 participants