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

Reorganize SVGs in the Project #121

Closed
endigo9740 opened this issue Aug 18, 2022 · 2 comments · Fixed by #122
Closed

Reorganize SVGs in the Project #121

endigo9740 opened this issue Aug 18, 2022 · 2 comments · Fixed by #122
Assignees
Labels
enhancement New feature or request help wanted Extra attention is needed ready to test Ready to be tested for quality assurance.

Comments

@endigo9740
Copy link
Contributor

Evaluate different conventions to use to better organize component and documentation SVG icons.

See my comment here talking about this in greater detail:
#118

@endigo9740 endigo9740 self-assigned this Aug 18, 2022
@endigo9740 endigo9740 added documentation Improvements or additions to documentation enhancement New feature or request labels Aug 18, 2022
@endigo9740
Copy link
Contributor Author

endigo9740 commented Aug 18, 2022

Some interesting gotchas with using SVG with an <img> tag, you can't style the interior contents of the SVG this way!

https://css-tricks.com/using-svg/

The problem with both img and background-image…
Is that you don’t get to control the innards of the SVG with CSS like you can with the following two ways. Read on!

I have no idea how I didn't already know this, probably one of those things I've learned but forgot. However, this means the idea of using that method kind of goes out the window.

@endigo9740 endigo9740 linked a pull request Aug 18, 2022 that will close this issue
@endigo9740
Copy link
Contributor Author

endigo9740 commented Aug 18, 2022

@niktek @thomasbjespersen

So I came up with a great solution for handling SVGs that I think bridges the best ideas from both Nik and myself.

The notable changes include:

  1. I've introduced a new standardized component called SvgIcon:
    https://github.com/Brain-Bones/skeleton/blob/dev/src/lib/SvgIcon/SvgIcon.svelte
  2. I've bundled all SVG path data into a single definition file called icons.ts:
    https://github.com/Brain-Bones/skeleton/blob/dev/src/lib/SvgIcon/icons.ts
  3. I've updated all instances of SVG icons throughout the project to utilize this new component.

The combines Nik's idea of leaning into a Svelte component, plus my idea of documenting in a single file, while creating a brand new component that follows the Skeleton conventions for props, including passthrough attributes, Tailwind styles, generic styles, and A11y support. Plus icon.ts allows us to add the missing Font Awesome attribution.

Additionally, this can still be used with standard SVG child content - you just drop any any code that would normally live between the <svg>...</svg> tags in as the default slot value.

I've documented the component here:
https://github.com/Brain-Bones/skeleton/blob/dev/src/routes/components/svg-icons/%2Bpage.svelte

Though please note this doc page is currently hidden from the nav bar. I would like to dogfood the component for a bit before we release it to the public. The biggest drawback is the canned set of icons are pretty limited (just what we use in components and docs). However, the ability to build your own is pretty nice. The fact the component comes with default sizes and styling is really nice. Plus there's a default icon shown if you don't provide a name or slot value.

This has been merged into dev if you would like to test it yourselves.

Screen Shot 2022-08-18 at 3 38 10 PM

Screen Shot 2022-08-18 at 3 38 15 PM

@endigo9740 endigo9740 reopened this Aug 18, 2022
@endigo9740 endigo9740 added ready to test Ready to be tested for quality assurance. help wanted Extra attention is needed and removed documentation Improvements or additions to documentation labels Aug 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed ready to test Ready to be tested for quality assurance.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant