hashicorp / flight

Archived. The flight repo now lives in the design-system monorepo
https://github.com/hashicorp/design-system
11 stars 4 forks source link

Placeholder "quest issue" to have ember-flight-icons consume flight-icons #108

Closed amyrlam closed 3 years ago

amyrlam commented 3 years ago

making an issue out of https://github.com/hashicorp/flight/pull/99, which is kinda moot rn as the icons have been updated

my ember discord post: https://discord.com/channels/480462759797063690/483601670685720591/879559675421532190

//

I would import them in the flight-icon.js component file. Something like: import icons from '@hashicorp/flight-icons/icons';

this didn't work for me, but then tried ember-auto-import (still in progress)

//

Couple of ideas:

  1. Have a build step in the project that relies on dist folder of@hashicorp/flight-icons, builds the sprite and then dumps the output of that into the addon's public folder so the existing behavior works as it does now. Pros is that it separates out responsibility for building the sprite away from the ember addon entirely and also removes the need to ship the sprite as part of @hashicorp/flight-icons
  2. Look at what ember-svg-jar does to read a directory of SVGs from a directory in node_modules. (looks like some broccoli stuff)

//

maybe this, idea from: https://github.com/ef4/ember-auto-import#usage-from-addons

broccoli example: https://github.com/minutebase/ember-inline-svg/blob/master/index.js

MelSumner commented 3 years ago

Might be useful - here are some examples of other ember addons that pull in other libs to use them in components:

ember-mdi ember-semantic-ui

ghost commented 3 years ago

@amyrlam a sprite sounds like it has utility beyond just the Ember use case.

amyrlam commented 3 years ago

@randallmorey 👍 for sure

i'll try to get another WIP PR up for this ticket to show what i mean soon. i discarded the previous one cause built off the 700 icon file change from the new export-go

amyrlam commented 3 years ago

just realized i misread brian's original comment about not including the sprite in @hashicorp/flight-icons. will follow up on the slack thread

MelSumner commented 3 years ago

@amyrlam so one thing that would block us from importing @hashicorp/flight-icons is that there are no icons in that package.

If you look at ember-mdi and trace where the icons are imported from, they come from here: https://github.com/Templarian/MaterialDesign-SVG

That's basically what @hashicorp/flight-icons would need to be for us to import that into the addon and use it as a dependency.

amyrlam commented 3 years ago

i think there is some confusion, all the .svg icons are in @hashicorp/flight-icons. (that was broken in 0.04-beta and 0.05-beta, so i deprecated those.) here is boundary using 0.06-beta https://github.com/hashicorp/boundary-ui/pull/721 and i can take a closer look at what Rose::Icon is doing (also pairing with randy tmrw)

amyrlam commented 3 years ago

flight-icons/ could contain all of the svg's (and optionally the sprite, although no one is using that right now)

ember-flight-icons/ could contain the sprite and doesn't need the 700 svg files

this could potentially be a step forward

amyrlam commented 3 years ago

@MelSumner not urgent, but want to follow up on your comment, i'm not sure what you mean could you elaborate?

If you look at ember-mdi and trace where the icons are imported from, they come from here: https://github.com/Templarian/MaterialDesign-SVG

image

looks like they read from a json file?

was also looking at this https://github.com/kaermorchen/ember-mdi/blob/master/addon/components/md-icon.hbs

i think a small step in the right direction for this issue, could be having flight-icons contain all the svg's, and ember-flight-icons the sprite file for now? but i'll make a PR and see what folks think

ghost commented 3 years ago

IMHO ember-flight-icons should not contain sprites or any other assets. It should merely be a glue layer between the raw assets and Ember. The sprite will probably have utility beyond Ember, which is why you might want it to live in the base repo. We can chat today in 1on1 but I see the requirements of ember-flight-icons as:

And this is totally an outsider's perspective so don't take it too seriously.

amyrlam commented 3 years ago

sounds good, we can talk about it when we meet

right now ember-flight-icons does depend on a sprite: https://github.com/hashicorp/flight/blob/main/ember-flight-icons/addon/components/flight-icon.hbs#L13 def open to changes, settled on that as a first iteration