-
Notifications
You must be signed in to change notification settings - Fork 22
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 licences of vendor icon libraries #167
Conversation
…d licence links (repo or general, depending on availability)
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #167 +/- ##
==========================================
- Coverage 87.02% 86.93% -0.10%
==========================================
Files 120 120
Lines 3107 3107
Branches 330 330
==========================================
- Hits 2704 2701 -3
- Misses 299 304 +5
+ Partials 104 102 -2 ☔ View full report in Codecov by Sentry. |
LICENSE
Outdated
* Foundation icons https://get.foundation/icon-fonts.html | ||
(MIT Licence https://github.com/thecreation/standard-icons/blob/master/modules/foundation-icons/LICENSE) | ||
* Open Iconic https://github.com/iconic/open-iconic | ||
(MIT Licence https://github.com/iconic/open-iconic/blob/master/ICON-LICENSE) |
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.
This seems to be wrong as well: MIT is for the icons only, but we are using the fonts itself, which are subject to the OFL-1.1: https://github.com/iconic/open-iconic/blob/master/FONT-LICENSE
@@ -26,11 +26,8 @@ | |||
"foundation-icons", | |||
"elegant-icons", | |||
"feather-icons", | |||
"happy-icons", | |||
"icomoon", |
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.
I'm hearing rumours that there is still something trying to load these.
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.
Any indications? They're still in the docs though...
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.
Well, I picked up this issue: django-cms/django-cms-quickstart#58 and traced the error back to this PR. Downgrading to 1.2.0
seems to resolve the bug. I have not gone deep into this yet, but I can still grep for files with the same name as those icon libraries: /djangocms_frontend/contrib/icon/static/djangocms_frontend/icon/vendor/assets/icons-libraries/icomoon.json
(not sure if that's relevant at the moment).
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.
This might be related to my findings in #166 (comment) from about a month ago, id est that some files still reference the old icon sets.
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.
@stefan6419846 It is! Thank you! The css files reference the removed fonts which breaks whitenoise. I have removed the css files a month ago. This needs to go through the release process now!
The json files are staying, so that people fine with the iconset's license can use them more easily.
Fix #166