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

[icon library] - add react icon names and download functionality to icon cards #766

Closed
jnm2377 opened this issue Jan 10, 2020 · 78 comments · Fixed by #906
Closed

[icon library] - add react icon names and download functionality to icon cards #766

jnm2377 opened this issue Jan 10, 2020 · 78 comments · Fixed by #906

Comments

@jnm2377
Copy link
Contributor

jnm2377 commented Jan 10, 2020

Summary

Icon library page currently only shows the regular name for icons, but we don't have anywhere that lists icons according to their icons-react name. i.e. Misuse24 CheckmarkFilled24, etc.

Desired outcome

overflow menu allowing "export of icons"

  • raw svg
  • download svg
  • react component

cc: @joshblack

@vpicone
Copy link
Contributor

vpicone commented Jan 13, 2020

I'll take this, the hold up on this previously was how we want to display it: tooltip vs button that copies to clipboard etc.

@vpicone
Copy link
Contributor

vpicone commented Jan 13, 2020

Ideally we'd have an overflow menu that would present:

  • React
  • SVG Download
  • Angular maybe?

We could also have the icons themselves be clickable to copy the React code (our preferred implementation)

@vpicone
Copy link
Contributor

vpicone commented Jan 13, 2020

@joshblack which size would it make sense to have copied here do you think? Maybe this would be a good time to implement the <Checkmark size='sm' /> api.

@joshblack
Copy link
Contributor

@vpicone my hunch would be 16 or 20, icons in a broader sense should be revisited so that we could do things like what you're suggesting.

If we want to, we could expose the module base name (like CheckmarkFilled) and share info about the sizes, but I totally understand if that'd be confusing.

For icon packages, most likely we'll want to only expose React as Angular/Vue do not have active attention dedicated to them (in fact Angular hasn't been published in the previous minors due to build issues)

@vpicone
Copy link
Contributor

vpicone commented Jan 13, 2020

@joshblack that makes sense, I think the only other thing we'd want is a download menu item then @laurenmrice

@chrisconnors-ibm
Copy link
Contributor

We do not want to provide a way to easily download SVGs from the icon page. This leads to orphaned icons in product endpoints that never catch iterations or updates.

@joshblack
Copy link
Contributor

How did we want to handle SVG downloads for presentations? I think that's one case that did come up recently for wanting to download them.

@chrisconnors-ibm
Copy link
Contributor

We could provide code snippets to call them from our packages, and maybe their menu path in the Sketch kit, but not do things that make it super easy for teams to do things we actively should be discouraging (SVG downloads).

How did we want to handle SVG downloads for presentations? I think that's one case that did come up recently for wanting to download them.

This is more of a pictograms use case, so maybe we do something different there. but the net is for a preso you can get them all from the repo as part of the package or you can dl the illustrator master file.

@chrisconnors-ibm
Copy link
Contributor

if you need just one from the repo, the raw button gets you https://raw.githubusercontent.com/carbon-design-system/carbon/master/packages/icons/svg/32/badge.svg

@chrisconnors-ibm
Copy link
Contributor

to summarize - no raw or download svg from the icon cards or pages please

@mjabbink
Copy link
Contributor

I think this is a must for pictograms. It’s a weekly request and even from my boss Terry and some other executives. They do not want to poke around in GitHub.

I’s also extend this functionality to UI icons as the same request comes up there as well but less frequent. Not all people use Sketch or even understand GitHub for that matter but want to use and have access to individual icons.

Holding them sacred also pushes people to do worse shit that I’ve seen equally or more distasteful.

Let’s please proceed with this. I understand the downsides but the entire team and a very large number of users will appreciate this.

@mjabbink
Copy link
Contributor

mjabbink commented Jan 17, 2020

So if i wasn’t clear. Please execute this issue.

@chrisconnors-ibm
Copy link
Contributor

If we do this for icons we will encourage people to do the easy thing (grab the SVG/DL the icon) rather than the right thing (access our icons via the packages, or the design kits) and the FIRST PERSON to notice and complain about teams using an icon we've iterated on in their applications (think: copy icon) will be you, @mjabbink. And you're going to be complaining a lot more then because they will never, ever, track down that SVG code and fix it.

We've already (and always) said "make direct DL possible for pictograms, because orphaning them doesn't matter", so please by all means add direct dl to these cards: https://www.carbondesignsystem.com/guidelines/pictograms/library

…but that should probably be a separate issue. Up to you all.

I suspect @designertyler has a spec for these types of functionality.

@chrisconnors-ibm
Copy link
Contributor

We should add a link somewhere to the .ai master file in the repo: https://github.com/carbon-design-system/carbon/blob/master/packages/icons/master/ui-icon-master.ai

@mjabbink
Copy link
Contributor

I’m more inclined to this for Pictograms. I’m not so sure holding back on icons is a real preventative measure against poor behavior. I also do not foresee adjustments to icons. I see new ones being added as we do plenty of that but date we have not modified many icons over the past year. Unless you consider the 16px ones you and Conrad did. I would be fine with first releasing this functionality with pictograms then icons later to test the waters.

@aagonzales
Copy link
Member

aagonzales commented Jan 23, 2020

While encouraging people to use icons through the Sketch library works if you're a Sketch user its starting to not scale as we introduce other tools, like Axure and Adobe XD. Right now the icons aren't available in those tools and accessing them from the website is easier than having to have a second tool (Sketch) or getting them from GitHub which many designers are uncomfortable with outside of issue creation. Sketch licenses are also hard to come by outside of design.

Not using the icon symbol in Sketch is the same as detaching a symbol, it only really hurts the person that does it. I think accommodating for these other usages by adding the download to the website will be more helpful than worrying about people not using the Sketch library.

@designertyler
Copy link
Contributor

designertyler commented Jan 23, 2020

I tried to summarize the requirements mentioned here so we can generate design specs. Let me know if we're missing any or if some should be a phase 1 or phase 2, 3, 4, etc.

Requirement 1: Add react icon names to icon card
As a product developer want to see the different sized React icon names so I can copy and paste the correct size into my project.

Requirement 1.2: Add angular icon names to icon card (?)
As a product developer want to see the different sized Angular icon names so I can copy and paste the correct size into my project.

Requirement 2: Enable direct download for pictograms
As a product designer I want to download the SVG asset of a specific pictogram so I can use it in my project.

Requirement 2.2: Enable bulk download for pictograms
As a product designer I want to select multiple Pictogram SVG's and download all at once so I don't have to repeat the same action to get all the pictograms for my project.

Requirement 3: Inform the preferred method to get icon assets
As a product designer I can learn how to get the icon asset in Sketch without directly downloading the SVG.

Requirement 4: Enable direct download for icons
As a product designer I want to download the SVG asset of a specific icon so I can use it in my project.

Requirement 4.2: Enable bulk download for icons
As a product designer I want to select multiple icon SVG's and download all at once so I don't have to repeat the same action to get all the pictograms for my project.

Requirement 5: Icon assets for non-Sketch users
As a product designer I want to get the SVG assets for an icon so that I may use the icon in my XD or Axure project.

@mjabbink
Copy link
Contributor

Please proceed with issue defined above. Meaning direct download and copy for both icons and Pictograms

@joshblack
Copy link
Contributor

Just a heads up @vpicone I'm updating our icon metadata structure to make it easier to parse for different meta fields, let me know if you want to sync up on this work 👍

@vpicone
Copy link
Contributor

vpicone commented Jan 23, 2020

@joshblack sounds good, a list of changes would suffice! we do the flattening of the metadata ourselves at run time at the moment.

Downloading svg icons and pictograms is a real use case, linking to the master file isn't a solution for things like keynote/powerpoint/non-sketch design tools. Not sure why pictograms and icons would be treated different.

@vpicone
Copy link
Contributor

vpicone commented Jan 23, 2020

@designertyler Requirement 4 and 5 seem the same. Icons and Pictograms are treated very similarly by the website so realistically things would be added in tandem.

@vpicone
Copy link
Contributor

vpicone commented Jan 24, 2020

@joshblack the biggest complaint for me is the sizes in the icon names at this point. I feel like a bit of a broken record for saying this, but these are scaleable vector graphics, they shouldn’t be bespoke sprites at individual sizes.

It’s not a blocker since we can always change what’s copied. However, having the copied code read Chevron16 or Chevron24 and making an assumption on which size they want (and assuming that they’d even know what that number means or what the other options are) is untenable to me.

I get that we had weird bespoke size constraints, but out of the dozen or so icon libraries in react I don’t see others taking this approach. It just really feels like what should be a prop encoded in the component name.

It causes issues with bundle size to boot. We should be accommodating the least common denominator of treeshaking. By exporting 4X the number of icons we need to be, ineffective treeshaking results in bombing bundle size.

Opened: carbon-design-system/carbon#5170

@joshblack
Copy link
Contributor

@vpicone I'm pretty sure we would need to constrain icon size no matter what, e.g. some assets will need to be used in a 16px context or some in a 20px context. This was the basis for the icon size tokens we explored end of last year. I believe the knowledge of when to use which is communicated between dev and design.

By exporting 4X the number of icons we need to be, ineffective treeshaking results in bombing bundle size.

The library is setup to be tree-shaken and one can confirm this in a fresh install of CRA. Times when this deopts are due to bundler config/settings/versions.

image

If you have steps to show how this deopts let me know 👍


but these are scaleable vector graphics, they shouldn’t be bespoke sprites at individual sizes.

I think one goal of the project would be to only maintain one asset per icon. What was observed was that certain assets rendered poorly on a wide breadth of devices/screens when drawn on 16px artboards. I believe this is what caused the situation that we have today. Could you share how this problem could be addressed?

@vpicone vpicone changed the title [icon library] - add react icon names to icon cards [icon library] - add react icon names and download functionality to icon cards Feb 5, 2020
@vpicone
Copy link
Contributor

vpicone commented Feb 5, 2020

blocked by carbon-design-system/carbon#5170

@vpicone
Copy link
Contributor

vpicone commented Feb 5, 2020

@designertyler it looked like you were about to drop a 🔥 spec for this.

#766 (comment)

@laurenmrice did you have something for it?

@mjabbink
Copy link
Contributor

This is my hack to see what it would look like

Screen Shot 2020-02-14 at 3 13 19 PM

@mjabbink
Copy link
Contributor

Can’t we assume </> will work in Carbon site?

@chrisconnors-ibm
Copy link
Contributor

@mjabbink - I am referring to the search and filter controls.

IDL:
image

CDS:
image

@chrisconnors-ibm
Copy link
Contributor

chrisconnors-ibm commented Feb 14, 2020

The headings are meant to help people figure out where they live in the plugin:
image

We could investigate other ways to convey that though.

@chrisconnors-ibm
Copy link
Contributor

@mjabbink

Can’t we assume </> will work in Carbon site?

I think in a meeting I wasn't part of @vpicone @jeanservaas maybe others thought if we use the react icon people won't be surprised that what they got was react code.

I was ok with </>.

@mjabbink
Copy link
Contributor

I’m ok with </> and prefer it over react atom unless that is really more clear.

@mjabbink
Copy link
Contributor

Also we went UI icons to be clear since we have several types of iconography. UI icons, app icons and Pictograms

@chrisconnors-ibm
Copy link
Contributor

chrisconnors-ibm commented Feb 17, 2020

this behavior not present @ preview
#766 (comment)
Specifically

When hovering/focusing on an individual action icon a tooltip will appear.
"Download SVG"
"Copy <icon-name />"

icon_card

cc @aagonzales @jeanservaas

@aagonzales
Copy link
Member

I think now that it has been deployed any outstanding issues or bugs need to be opened in a new issue.

@mjabbink
Copy link
Contributor

This is meant for IDL as well and Pictograms

@mjabbink mjabbink reopened this Feb 17, 2020
@aagonzales
Copy link
Member

aagonzales commented Feb 17, 2020

[the issue that will never be closed]

FYI this is the carbon-website repo, it may need to be moved or a new issue opened for IDL.

@vpicone
Copy link
Contributor

vpicone commented Feb 17, 2020

@mjabbink the PR you just closed in IDL is adding them there (carbon-design-system/design-language-website#439)

Pictograms were included in the initial PR and are currently live: https://www.carbondesignsystem.com/guidelines/pictograms/library

@vpicone vpicone closed this as completed Feb 17, 2020
@chrisconnors-ibm
Copy link
Contributor

I thought Pictograms (IDL and CDS) and Icon cards (IDL) were only supposed to get the DL button:
#766 (comment)

@mjabbink
Copy link
Contributor

mjabbink commented Feb 17, 2020

@chrisconnors-ibm Yes, download button for those only

@vpicone
Copy link
Contributor

vpicone commented Feb 17, 2020

@mjabbink @chrisconnors-ibm It seems like pictograms would be more requested for use in keynote/outside of our libraries. Is there a reason we wouldn't want folks to download it?

@mjabbink
Copy link
Contributor

@vpicone yes, pictograms and icons are to have download functionality

@chrisconnors-ibm
Copy link
Contributor

chrisconnors-ibm commented Feb 17, 2020

I thought Pictograms (IDL and CDS) and Icon cards (IDL) were only supposed to get the DL button:
#766 (comment)

The question is whether either IDL Pictogram cards or IDL Icon Cards get the code </> action.

@aagonzales suggested leaving the code </> action for CDS Pictogram cards; if there's utility for for Carbon FEDs in that, we should leave the code button on Pictogram Cards on CDS.

@mjabbink
Copy link
Contributor

go for it.

@mjabbink
Copy link
Contributor

I thought that saying DL only for icons and pictograms was clear. But if you all think </> is useful in Idl for pictograms then do it. I was thinking it was not but I based that on my own opinion only.

@mjabbink
Copy link
Contributor

Could have both really even if the </> is hardly ever used.

@vpicone
Copy link
Contributor

vpicone commented Feb 18, 2020

@chrisconnors-ibm we have the pictograms-react library, not sure why we wouldn't have the copy code button for them on CDS.

@mjabbink Right now CDS has both buttons for both libraries, IDL has just download for both libraries. Is that not the intent?

@mjabbink
Copy link
Contributor

mjabbink commented Feb 18, 2020

We can have the libraries be identical I guess. Not harm to have copy code in IDL and I’m all for making them be exactly the same. So strike what I said earlier to be just DL function only. I’m with the other old man.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants