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

Changed ML team infra plots to .svg #6

Merged
merged 3 commits into from
Jan 16, 2025
Merged

Conversation

BoBer78
Copy link
Contributor

@BoBer78 BoBer78 commented Jan 15, 2025

No description provided.

@BoBer78
Copy link
Contributor Author

BoBer78 commented Jan 15, 2025

I think I might still need to change the background of the schemes (or change the text color). Please tell me before merging ! @GianlucaFicarelli

@GianlucaFicarelli
Copy link
Collaborator

I think I might still need to change the background of the schemes (or change the text color). Please tell me before merging ! @GianlucaFicarelli

Yes I see that there are also other svg diagrams with transparent background and dark text, but the text isn't very readable when using the dark theme in GitHub.

I'd set a white background and dark/black text. Not the best visually for dark themes, but at least it's always visible.

Alternatives, not considered for the moment:

  • provide 2 images for each graph, one for dark and one for light theme (github should allow to specify which one to use, but we would need to duplicate all the graphs)
  • use a transparent background and a custom css to apply an invert filter (it works with the html produced using mkdocs, but I don't think it can work directly on github)

@BoBer78
Copy link
Contributor Author

BoBer78 commented Jan 16, 2025

I changed the background to be white with black text. We can iterate in a more complex version compatible with dark mode if needed.

@GianlucaFicarelli GianlucaFicarelli merged commit 3c282be into main Jan 16, 2025
2 checks passed
@GianlucaFicarelli GianlucaFicarelli deleted the ml_schemes_to_svg branch January 16, 2025 10:39
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.

2 participants