carbon-design-system / icons-motion

A library of animated Carbon icons
Apache License 2.0
17 stars 10 forks source link

Svg titles and fillColor prop #232

Closed kristastarr closed 1 year ago

kristastarr commented 1 year ago

Updates to the Idea icon which were prioritized because they are being used in production right now and the removal of the svg <title> is necessary... these changes will be applied to additional icons in the future

kristastarr commented 1 year ago

@elycheea @glapadre @tay1orjones Could one of y'all review this when you have time please? πŸ™

netlify[bot] commented 1 year ago

Deploy Preview for carbon-icons-motion ready!

Name Link
Latest commit 55c30783b217680e0eb828539c077be547255f73
Latest deploy log https://app.netlify.com/sites/carbon-icons-motion/deploys/6446ad5a57abec0008ab32a9
Deploy Preview https://deploy-preview-232--carbon-icons-motion.netlify.app/
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

elycheea commented 1 year ago

@kristastarr Don’t forget to look into the package-lock!

kristastarr commented 1 year ago

@elycheea @tay1orjones I made the update to require one title prop instead of two separate ones for needsTitle and customTitle. Updated propTypes as well. Now there is a default title (the icon's name) if not otherwise specified.
I don't think this is the most eloquent solution but it's working. Is it okay with y'all if I go ahead and merge it, and can improve on it later?

kristastarr commented 1 year ago

@elycheea @tay1orjones I made the update to use one title prop instead of two separate ones for needsTitle and customTitle. Updated propTypes as well. Now there is a default title (the icon's name) if not otherwise specified.
I don't think this is the most eloquent solution but it's working. Is it okay with y'all if I go ahead and merge it, and can improve on it later?