Conversation
There was a problem hiding this comment.
I took the liberty to also expand a bit the CONTRIBUTING file
| root_element = generate_style_element("root.css") | ||
| if root_element is None: | ||
| return all_styles | ||
| return [root_element, *all_styles] |
There was a problem hiding this comment.
This is how I add the new root.css file to the other styles.
| style = Element("style") | ||
| style.text = f"<![CDATA[\n{css_content}\n]]>" | ||
| return style | ||
| return generate_style_element(css_file_name=f"css_{self._css_class}.css") |
There was a problem hiding this comment.
Refactored this slightly with the introduction of generate_style_element to be able to reuse the CSS loading for root.css
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
|
||
|
|
||
| def generate_style_element(css_file_name: str) -> None | Element: | ||
| css_file_path = Path(f"src/ewoksdraw/css_styles/{css_file_name}") |
There was a problem hiding this comment.
It was already the case before, but don't we risk having issues if the working directory changes?
canvas = SvgCanvas(10, 10)
canvas.add_element(background)
os.chdir('/somewhere')
canvas.draw(Path('/tmp/example.svg'))We could use importlib
from importlib.resources import files
css_file = files("ewoksdraw").joinpath("css_styles", css_file_name)There was a problem hiding this comment.
You are right, I thought about it afterwards. We should instead rely on the directory of the package itself.
| stroke-width: 0.2%; | ||
| fill: var(--transparent); | ||
| stroke: var(--stroke-color); | ||
| stroke-width: var(--stroke-width); |
There was a problem hiding this comment.
--stroke-width is now shared between css_task_box and css_task_line.
The values were different prior to the change (1 vs 0.2 %). Should we create a separate variable?
There was a problem hiding this comment.
Ah yes. Well, it depends: do we want the box and the line to have both the same stroke width or not?
I'd think it would be more consistent to have the same but perhaps you had something else in mind when you designed it earlier?
There was a problem hiding this comment.
Let's keep it like that for now.
Anyway, if we want to tone it down an easier way, it is to change the color.
|
I agree with you, and I wanted to do that in the first place, but I was struggling with HTML integration. Do we have to support SVG strings directly embedded in HTML? An example of an HTML page highlighting the issue: svg-leak.html> This can be fixed by adding the style variable directly in the element. I'm 100% sure there is a better way to do that. |
Only if the page uses the same CSS variables. So there is two ways of fixing this:
Perhaps the second option is cleaner. What do you think? |
|
Yes, I'm in favor of solution 2, making the SVG block self-contained. |
|
Please have a second look @LudoBroche, I added two commits to handle the issues you pointed out |
2 participants
PR summary
In web projects, we usually keep color consistent by using CSS variables
See https://github.com/silx-kit/h5web/blob/828da78669af0fa56e6cb9a9dd54cbfc138c5357/packages/app/src/App.module.css#L12 for example.
It thought it would be good to do it for our produced SVG as well. We can then keep all colors in a single place so that it is easier to change the style conditionally. This is mainly for #22 but we could imagine providing different theming for the output later.
For this, I added a new
root.cssfile containing the variables that is always included in the generated SVG.AI Disclosure