Skip to content

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.


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