-
Notifications
You must be signed in to change notification settings - Fork 71
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add LMS icons #3029
feat: add LMS icons #3029
Conversation
✅ Deploy Preview for paragon-openedx ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
7833bd9
to
48e51cf
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3029 +/- ##
=======================================
Coverage 93.18% 93.18%
=======================================
Files 249 249
Lines 4342 4342
Branches 1036 1036
=======================================
Hits 4046 4046
Misses 292 292
Partials 4 4 ☔ View full report in Codecov by Sentry. |
> | ||
<path | ||
d="M7.5 22C6.53333 22 5.70833 21.6583 5.025 20.975C4.34167 20.2917 4 19.4667 4 18.5V5.5C4 4.53333 4.34167 3.70833 5.025 3.025C5.70833 2.34167 6.53333 2 7.5 2H20V17C19.5833 17 19.2292 17.1458 18.9375 17.4375C18.6458 17.7292 18.5 18.0833 18.5 18.5C18.5 18.9167 18.6458 19.2708 18.9375 19.5625C19.2292 19.8542 19.5833 20 20 20V22H7.5ZM6 15.325C6.23333 15.2083 6.475 15.125 6.725 15.075C6.975 15.025 7.23333 15 7.5 15H8V4H7.5C7.08333 4 6.72917 4.14583 6.4375 4.4375C6.14583 4.72917 6 5.08333 6 5.5V15.325ZM10 15H18V4H10V15ZM7.5 20H16.825C16.725 19.7667 16.6458 19.5292 16.5875 19.2875C16.5292 19.0458 16.5 18.7833 16.5 18.5C16.5 18.2333 16.525 17.975 16.575 17.725C16.625 17.475 16.7083 17.2333 16.825 17H7.5C7.06667 17 6.70833 17.1458 6.425 17.4375C6.14167 17.7292 6 18.0833 6 18.5C6 18.9333 6.14167 19.2917 6.425 19.575C6.70833 19.8583 7.06667 20 7.5 20Z" | ||
fill="#707070" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The hardcoded colors in the svgs go away once the icons are generated
paragon/icons/svg/lms_book_complete.svg
Line 16 in 48e51cf
fill="#0D7D4D" |
vs
paragon/icons/jsx/LmsBookComplete.jsx
Line 13 in 48e51cf
fill="currentColor" |
so I wasn't concerned about the svgs having hardcoded colors. If we feel a need to remove those we can.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, this is not a problem for me, I just decided to clarify)
6e41659
to
f1c2014
Compare
@brian-smith-tcril If we don't need any additional reviewers, it looks like ready for merge. |
Based on the conversation from today's working group meeting https://openedx.atlassian.net/wiki/spaces/BPL/pages/4179132431/2024-04-17+Meeting+notes I will update the icons to not have hardcoded white for the checkmarks in the "complete" icons |
After a bit of investigation I found the circle with the checkmark is simply covering up the rest of the icon, and relies quite heavily on the fill and stroke. I'm not very experienced with advanced clip masking in svg, if someone else is more experienced and willing to take a stab at it that would be wonderful, if not I can do a deep dive into it tomorrow. |
Co-authored-by: ihor-romaniuk <ihor.romaniuk@raccoongang.com>
f1c2014
to
2f68e54
Compare
🎉 This PR is included in version 22.3.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
fill: "currentColor" | ||
})); | ||
}; | ||
export default SvgLmsEditsquare; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@brian-smith-tcril In this PR, you committed some files with names that differ only in case:
icons/es5/LmsEditSquare.js
andicons/es5/LmsEditsquare.js
icons/es5/LmsEditSquareComplete.js
andicons/es5/LmsEditsquareComplete.js
- same files in the
jsx
folder
This is causing all kinds of havoc on my Mac where the file system is case-insensitive and git keeps toggling back and forth between both versions of the file with a single name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Description
Resolves #3024
Deploy Preview
https://deploy-preview-3029--paragon-openedx.netlify.app/foundations/lms-icons/
Merge Checklist
example
app?wittjeff
andadamstankiewicz
as reviewers on this PR.Post-merge Checklist