Skip to content

fix: stroke omitted for hi_outline_icons.rs #63

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

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

Conversation

goosethedev
Copy link

Solves #62

Omits the stroke attribute on codegen. I only tested it for the Heroicons files (the Material repo was too big for me to clone), but I assume it is also beneficial to avoid hardcoding the attribute in other files. Let me know if I should test it for the other files as well.

Also, I think it would be better to set stroke: "currentColor" to explicitely allow stroke overwriting by the parent, but it seems it isn't totally necessary.

@marc2332 marc2332 self-requested a review January 22, 2025 15:56
@nissy-dev nissy-dev requested a review from Copilot May 12, 2025 01:31
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR addresses issue #62 by omitting the stroke attribute from the generated icon files to allow for parent-based stroke overwriting.

  • Excludes the "stroke" attribute in the generated code for SVG child elements.
  • Aligns attribute filtering for icon file generation with the desired behavior for Heroicons.

Copy link
Member

@nissy-dev nissy-dev left a comment

Choose a reason for hiding this comment

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

Sorry for the delayed response.

It seems that in Heroicons, not only the stroke attribute, but also stroke-linecap, stroke-linejoin, and stroke-width are included in the returned HTML from child_elements.

stroke: "#374151",
stroke_linecap: "round",
stroke_linejoin: "round",
stroke_width: "2",

In this case, we need to omit those attributes and adjust the settings for extract_svg_colors, extract_stroke_linecap, and extract_stroke_linejoin accordingly.

fn extract_svg_colors(icon_prefix: &str) -> (&str, &str, &str) {
match icon_prefix {
"Fi" => ("\"none\"", "user_color", "\"2\""),
"Ld" => ("\"none\"", "user_color", "\"2\""),
"Io" => ("user_color", "user_color", "\"0\""),
_ => ("user_color", "\"none\"", "\"0\""),
}
}
fn extract_stroke_linecap(icon_prefix: &str) -> &str {
match icon_prefix {
"Ld" => "round",
"Fi" => "round",
_ => "butt",
}
}
fn extract_stroke_linejoin(icon_prefix: &str) -> &str {
match icon_prefix {
"Ld" => "round",
"Fi" => "round",
_ => "miter",
}
}

@goosethedev
Copy link
Author

I'll work on this in the upcoming days and check if other icon packs have similar problems.

@goosethedev
Copy link
Author

@nissy-dev while taking a closer look at this, I noticed some other aspects of the Heroicons:

  • The pinned submodule revision is 3 years old, missing several icons.
  • The Heroicons repo includes an optimized directory with modified versions of the src icons. Here, attributes follow more sensible values (including the stroke set to currentColor).
  • The latest version also includes two additional icon subsets for smaller icons (solid versions only): 16 (mini), 20 (micro).

I was thinking of turning this PR into a draft that solves all of these problems. I would:

  • Update the submodule to the latest versions.
  • Switch the SVGs path to the optimized directory.
  • Add two output Heroicons icon packs, making them four in total: hi_outline_icons.rs, hi_solid_icons.rs, hi_mini_icons.rs and hi_micro_icons.rs, same as shown in the Heroicons page.

Let me know what you think about this :)

@goosethedev
Copy link
Author

Also, unrelated to this PR, I was considering other modifications as separate PRs:

  • Add command line options (maybe using clap) to the codegen crate to run it only for a selected icon pack (right now it generates all icon packs, and the ones not downloaded in the icon_resources output empty files). e.g. cargo run -- heroicons.
  • Separate pipeline transformations for each icon pack for better modularity, so changes in one icon pack don't affect the other ones. Maybe a IconParser trait with a .create_icon_file?

@nissy-dev
Copy link
Member

Thank you for doing your work! I think your suggestions look good overall.

Update the submodule to the latest versions.
Switch the SVGs path to the optimized directory.
Add two output Heroicons icon packs, making them four in total: hi_outline_icons.rs, hi_solid_icons.rs, hi_mini_icons.rs and hi_micro_icons.rs, same as shown in the Heroicons page.

If possible, I’d appreciate it if you could split these changes into separate PRs.

Also, unrelated to this PR, I was considering other modifications as separate PRs:

This one looks good as well. Again, if it’s a major change, it would be great if you could split it into separate PRs before submitting.

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