skyglobal / skycons

A collection of brand approved icons for use across Sky.com
http://skyglobal.github.io/skycons/
BSD 3-Clause "New" or "Revised" License
6 stars 5 forks source link

Bug/separate dist files from demo #13

Closed ekynoxe closed 9 years ago

ekynoxe commented 9 years ago

We've updated only recently to the latest version, but one thing I hadn't noticed is that the generated "dist" files contain most of the css for the demo page as well. I've moved all demo-specific css, including color and hover states to the demo.css file.

Also changed the font size to use rems rather than ems, since when use in context, it massively confuses the css calculating the font-size, especially when applying it on icons that have both the .skycon and .skycon-- selectors. Having rems ensures that we don't end-up with * 1.125 * 1.125.

Also updated the Sky SVG to remove padding around it, but I believe all other icons should receive the same treatment. Positioning of icons should be left to whomever uses them, and no padding should be in any of them, unless they are use to properly align icons of the same set. I'm specifically thinking about the chevrons here, which are completely out of vertical alignment when rotated.

peter-mouland commented 9 years ago

hey, the code you talk about isn't demo-only code. It's used to give the skycons colour either by default or on hover. This is handy to make sure the remote-record button is always the correct red for example

ekynoxe commented 9 years ago

No, not only. It's bringing in a truck-load of sky fonts and css properties specific only to the demo page. We can bring the colour code back in, but that's a very small part of the pull request. We discovered all sorts of problems today migrating to the latest version because the dist css now incorporates a lot of styles not specifically targetted at skycons classes.

I can move the colors back in the skycons.css file, but have a look at the other changes please.

ekynoxe commented 9 years ago

How do I re-open this pull request by the way? I want it to still be visible as open for discussion, as it should be.

peter-mouland commented 9 years ago

ok - i think i know what has happened. We move to use a 'core' component which includes colours, but also typography. So the Sass should not 'import core' 'but import core/colours'

peter-mouland commented 9 years ago

good spot with the larger than needed css size, i've fixed this now and release 0.3.3. With the 'em', this is only on the line-height, which i in relation to the font size, so i think it fits in this case. i.e you don't want line-height to be rem, because if the font-size changes for that icon, the line-height wouldn't.

ekynoxe commented 9 years ago

Right, I'll have a look at 0.3.3. I'm pretty sure there will still be an issue with the news site as the line-height was wrecking some of our styles that didn't need it previously. I'd just like to avoid having overrides only because there is a default style pulled in with the bower component, when all we're really interested in are the actual font files and glyphs. Most, if not all of the icons we use on news (and I understand that's only one use for the skycons) are not inline with anything but in their own buttons, or detached from any content, so the line height is something we set ourselves in the style sheets of the blocks using the skycons.

That's also why I had removed the colour from the generated dist file, as in my view this should not be part of what the component provides.

ekynoxe commented 9 years ago

Would generating a dist file that only contains font glyphs and @font-file make sense? As an extra. User who want all the colours can use them with the current file, the ones who don't want all the colours and extras could have a no-colour.scss or no-fancy.scss or something like that. Just an idea

peter-mouland commented 9 years ago

yeah - i'm thinking the same thing... would be no over head and would not be a breaking change

ekynoxe commented 9 years ago

Right, I'll try and do that tomorrow, so most of the changes that were suggested in this PR would get there. Then we would use that in the news site. Great!

ekynoxe commented 9 years ago

All this has been fixed through other PRs and updates (literally, in 0.3.3 and 0.3.4). Closing.