Financial-Times / o-icons

Icons for FT sites. http://registry.origami.ft.com/components/o-icons
4 stars 4 forks source link

V5 #28

Closed alicebartlett closed 8 years ago

alicebartlett commented 8 years ago

This is the breaking change to o-icons.

This version removes the source icons, using the fticons repo instead. Things I'm not sure about:

That's all!

onishiweb commented 8 years ago

in order to build the stylesheet for classes like o-icons--book, i've had to read a file from inside the bower components dependencies. Is it safe to do this? I mean, is it safe to always assume this will be here? And not break un unexpected ways?

This should be fine because it's built using the gulp task which is run manually. If the file's not there, people can fix that with bower install and that's part of installing a module anyway so I don't see that being a problem. Only problem would be if we change the architecture of fticons, but surely then we could update those fixes here too... (hence locking fticons to a version).

in order for $o-icons-use-local-assets to work I've had to do something similar, setting the path to bower_components/fticons/svg/#{$icon-name}.svg. Is this safe? Will these icons always be here?

Could this be solved with o-assets? (I can't remember where o-assets sits in the process.) Otherwise I think this is fine.

I've removed the registry demo entirely, opting instead for a hidden 'test' demo so that if you're updating o-icons you can run it and check that everything still more-or-less works. Is there a user facing demo we should include for this, or should we leave that to the image set?

I think this is fine, we should add a link in the description to fticons as we talked about before this will also move from Primitives to Utils so it's probably ok that it doesn't have a visual demo.

Looking into the Readme now.

alicebartlett commented 8 years ago
onishiweb commented 8 years ago

Ok I don't think that should be a problem then, assumed that was the way o-assets was meant to be used.

Readme looks 👍

alicebartlett commented 8 years ago

OK, this is ready for a final review. I'll wait until Monday to merge/release anyway.