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 svgbob diagrams in dark mode #28

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

hmn53
Copy link
Contributor

@hmn53 hmn53 commented Sep 22, 2023

Closes #22

Currently, the SVG text elements receive the default color, black, which is not readable on dark mode.

This PR adds a one-liner CSS fix to apply the foreground css variable (var(--fg)) to the svg text element to make svg text legible and visible.

Copy link
Collaborator

@mgeisler mgeisler left a comment

Choose a reason for hiding this comment

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

Thanks, I tested it locally and it works great.

@mgeisler
Copy link
Collaborator

When I downloaded it, I got

Compiling proc-macro2 v1.0.67

and the test failure is with

Compiling proc-macro2 v1.0.47

I guess this is because of the Cargo.lock file?

@hmn53
Copy link
Contributor Author

hmn53 commented Sep 22, 2023

Yes, that seems to be the issue.

@mgeisler
Copy link
Collaborator

Yes, that seems to be the issue.

I would suggest making a separate PR that updates the Cargo.lock file so we can get CI running again 😄 Then rebase this on top and we should be back in business.

@@ -16,5 +16,5 @@ pub fn bob_handler(s: &str, settings: &Settings) -> String {
svg.render_with_indent(&mut source, 0, true).expect("html render");

let style = Style::new("svg { width: 100% !important; }").set("type", "text/css");
format!("<div style='width:100%; height:{}px;'>{}{}</div>", height, style, source).replace('\n', "")
format!("<div style='width:100%; height:{}px; fill:var(--fg);'>{}{}</div>", height, style, source).replace('\n', "")
Copy link
Collaborator

Choose a reason for hiding this comment

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

I just noticed that there is a number of configuration options — could it be that we just need to update the default for fill_color?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updating the default for fill_color does not work for some reason.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Okay, thanks for checking! Then I just think you need to rebase this PR on top of master and that will make the tests pass.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@boozook, do you know more about or are you fine with just adding the CSS here?

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.

Diagrams don't use theme colors
2 participants