search

BitcoinDesign / Bitcoin-Icons

Icons made for Bitcoin applications, free to use.
https://bitcoinicons.com/
Other
107 stars 21 forks source link

Displays issues with optimized outline SVGs #51

Closed sbddesign closed 3 years ago

sbddesign commented 3 years ago

The react and vue components for outlined icons are displaying strangely for me.

Screen Shot 2021-07-26 at 8 46 39 AM Screen Shot 2021-07-26 at 8 49 13 AM

When I reviewed #48, I was doing a lot of testing with the filled components and forgot to check the outline components. 🙁

Having done a little detective work, I have found that the /optimized/outline SVGs seem to look good, whereas the /vue/outline SVGs have the display problems.

Can anyone else replicate this problem on their end? I simply looking at the raw output from running npm prepublishOnly.

@johncantrell97 flagging this for you since you know this well 🙂

johncantrell97 commented 3 years ago

So the 'issue' is definitely in the /optimized/outline svgs and not just the library components. If you look at the SVGO (the tool we are using to optimize/cleanup the svg files) configuration file for outline we are adding a stroke of the currentColor to all of the svgs:

 - 'addAttributesToSVGElement':
      attributes:
        - 'stroke': 'currentColor'

This is what is causing them to look weird but I think it's just surfacing a problem with how the outline SVGs are being created.

This is typically how people manage what color the icons are but this is showing that if you try to set stroke color it will cause distortion to the actual icon.

For now simplest solution is to just remove those 3 lines above from the svgo.outline.yaml but I'd make sure you can still easily control the color of these icons without having the same issue.

sbddesign commented 3 years ago

Thank you -- and actually, I concur that the issue is in /optimized/outline; I think I got those directory names swapped when I was copy/pasting them. I will try this out today and see what the results are.

GBKS commented 3 years ago

I can update the source icons in Figma and re-export them to avoid this issue. Could you please let me know how you set up those demo pages? I have no clue how to work with this new set up for the modules in the repo.

GBKS commented 3 years ago

And I may have accidentally published the vue module. I thought I could just test it based on this comment:

You can also run it locally after npm install to try it out and it will generate optimized svgs using svgo and then generate react + vue components into the corresponding directories.

sbddesign commented 3 years ago

@GBKS No big deal, we'll fix in the next publish. It's numbered as a pre-release anyways!

For future reference, npm run prepublishOnly will generate the package locally. You can then move to another directory with a Vue project and run npm install <local directory path of Bitcoin-Icons/vue> to test out the module locally. Then npm publish when ready for the package to be available on NPM.

sbddesign commented 3 years ago

I have been investigating this problem further. The suggestion from @johncantrell97 did not by itself fix the problem, however it was very illuminating for me and helped me to see more clearly the nature of the issue. Essentially, there are inconsistencies with how the icons are being exported out of Figma (stroke placement, fills, booleans, etc).

Ideally, I'd like to automate this as much as possible; I don't want us having to do a ton of manual flattening work on every icon, and I think it's good they remain in an editable state. So, I am in-progress making adjustments to the Figma plugin. I have been adjusting it to perform some post-processing on the vectors when generating the export page so that they will be more uniform. @GBKS I will submit a PR when the new code is ready for review.

GBKS commented 3 years ago

Thanks for looking into this. Beware of automatically adjusting the icon vectors, as they might turn ugly. Bosch and I already did a bunch of work to get clean exports, like only using center-aligned strokes, I had hoped we'd be in good shape there. My approach to having source and export versions of the icons was for each icon to include both, but hide the source group so it doesn't get exported.

GBKS commented 3 years ago

@sbddesign can you let me know what those issues were you found? I'd like to get them fixed.

sbddesign commented 3 years ago

I'll have to put that together for you. There were numerous little issues, so instead of building a list I was working on a script to fix it. I will try to get this script finalized or document a list of things that need fixed for you ASAP. Sorry for the delay.

GBKS commented 3 years ago

Sorry, can you please also provide step-by-step instructions on how to test this? I just tried without success:

I obviously don't understand how this system works.

GBKS commented 3 years ago

Here's video update to the progress on this. It requires both an update to the Figma plugin, and tweaks to many of the icons.

GBKS commented 3 years ago

Here's a new PR #57 with the fixed icons. Could you please test if they show up correctly in your test apps?

GBKS commented 3 years ago

Rebuilt the bitcoinicons.com site in Vue 3, using a locally pre-published version of the Vue module. All works smoothly. PR is here: https://github.com/GBKS/bitcoinicons.com/pull/6

I am trying to systematically work through this process:

Seems like a really fragile process, so I'd appreciate help with testing and ensuring this all works.

GBKS commented 3 years ago

Published the Vue module and implemented it on a bitcoinicons.com repo PR, see the preview.

This is in good shape now. I'll try to wrap it all up next week. Most likely I'll need help with React.

GBKS commented 3 years ago

Alright, did a few more fixes and also got the React module working. 0.1.6 is the current module version, you can get the react one here.

The live version of bitcoinicons.com also now uses the Vue module, making it easier to verify the icons render correctly.

Would be sweet if someone could verify that all works as desired now. Once that has happened and we've closed this issue, I'd like to revise the documentation with the latest changes, and maybe we can get started with that lookup table.

sbddesign commented 3 years ago

I've tested in React and Vue, it works correctly. I do have some nitpicks about certain icons, but I'll open those as separate issues. I feel like this framework is working now.

sbddesign commented 3 years ago

Thanks for all of this work @GBKS 🙏

GBKS commented 3 years ago

@sbddesign awesome, thanks for testing. There are some icon changes I was not happy with, but made more for the sake of getting export and rendering working properly. As those were fairly minor, I didn't want to hold things up further. Please open up those issues as you see fit.