Skip to content

feat(icon): expose 'icon-inner' and 'svg' as shadow parts #1027

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

Closed
wants to merge 1 commit into from

Conversation

Juarrow
Copy link

@Juarrow Juarrow commented Dec 4, 2021

For #960

  • expose .icon-inner and the SVG element as shadow parts ```::part(icon-inner)&::part(svg)``
  • add `examples to index.html
  • update icon/readme.md

(To be honest: this feels kind of hacky to me, so i opened this for discussion. Sorry for the inconvenience - if it causes any.)

Also
- add usage examples to index.html
- update icon readme
@Juarrow Juarrow force-pushed the 960_shadow-part_inner branch from 0c54b3a to b572c8d Compare December 8, 2021 20:58
@Juarrow Juarrow changed the title feat(icon): expose 'inner-html' and 'svg' shadow parts feat(icon): expose 'icon-inner' and 'svg' as shadow parts Dec 8, 2021
@christian-bromann
Copy link
Member

christian-bromann commented Apr 16, 2025

@Juarrow thanks for raising this PR. I am a bit concerned about the way we tweak the DOM through direct DOM writes instead of relying on the shadow DOM. Why can't we just make the following change:

-           <div class="icon-inner" innerHTML={this.svgContent}></div>
+           <div class="icon-inner" part="inner-icon" innerHTML={this.svgContent}></div>

?

@Juarrow
Copy link
Author

Juarrow commented Apr 17, 2025

@christian-bromann

It's been ~3.5 years and I don't use Ionic anymore. (And there probably have been many changes to Ionic itself, I guess - so no clue if this MR is still up-to-date enough.)

If you want you may close it, update it or just use it as inspiration.

@christian-bromann
Copy link
Member

Thanks for the response, I will go ahead and close this. Happy to re-open anytime.

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