Closed birtles closed 6 years ago
And yes, the icons will need explicit width/heights, or we get validation errors (which you can see at npm run test
).
And yes, the icons will need explicit width/heights, or we get validation errors (which you can see at npm run test).
And also because the icons are meant to be only used at their specific size even if you might scale the SVGs. The main reasons: icon-size consistency - all the icons are 16x16. And pixel snapping - being drawn at 16x16 we made sure that the stroke is snapped to the pixel grid. If we would scale the icon to 20x20 for example the two pixel stroke would be something like 2,xxx.
And yes, the icons will need explicit width/heights, or we get validation errors (which you can see at npm run test).
Great, I'll fix that. I didn't know about npm run test
.
I'll make the changes tomorrow.
I've addressed all the review feedback and added an extra commit that renames "Preferences Macos" to just "Preferences".
@birtles thanks for all the updates 🙌 yesterday we forgot to address this concern of yours:
Updates the "undock" icon so that it matches the Photon "Open in New" icon but has the same proportions as the other docking icons
We tested both variants and we don't think it's necessary to create a slightly different icon to match the others icons ratio. If that would be true we would need to adjust many icons in many different context which is not ideal. Having icons in different ratios helps users quicker scan the content of a component.
Ok. It looks a bit clumsy to me, but I think once we arrange these icons in a row in bug 1474223 it will be fine. So, let's not add this icon to the repo for now.
Ok, I've updated the PR. Please take a look.
This reflects the changes from bug 1474224.
In particular:
These icons don't include an explicit width/height since that's generally an anti-pattern when creating SVG resources (they should scale to their container) but perhaps it's needed for the Photon icon viewer though?