PR: Update FontAwesome and Material design icons #120
Conversation
There was a problem hiding this comment.
My comments:
-
Why did you remove
fontawesome5-brands-webfont.ttf? -
Please restore the file properties of
materialdesignicons-webfont.ttf(you added it as an executable file). -
To fix the errors in our tests, you also need to update the md5 font hashes here:
https://github.com/spyder-ide/qtawesome/blob/master/qtawesome/iconic_font.py#L36
Sorry, I just saw that you didn't remove it. Please forget about this comment. |
|
Great job @steff456! One last thing: please update the link to the Material design version you updated to in our Readme: https://github.com/spyder-ide/qtawesome/blame/master/README.md#L64 |
There was a problem hiding this comment.
docs/source/usage.rst also has an incorrect total for MDI icons (but github won't let me comment on unchanged lines.) Can you fix it please?
And what happened with fontawesome5-brands-webfont-charmap.json's diff? Did you change the line endings of the file? Contents are fine but diff shows as if you removed and added the same lines... Weird.
|
|
||
| - `fa` is the legacy [FA 4.7 version with its 675 icons](https://fontawesome.com/v4.7.0/icons/) but **all** of them (*and more!*) are part of FA 5.x so you should probably use the newer version above. | ||
|
|
||
| - `ei` prefix holds [**Elusive Icons** 2.0 with its 304 icons](http://elusiveicons.com/icons/). | ||
|
|
||
| - `mdi` prefix holds [**Material Design Icons** 3.0.39 with its 3039 icons.](https://cdn.materialdesignicons.com/3.0.39/) | ||
| - `mdi` prefix holds [**Material Design Icons** 3.8.95 with its 3039 icons.](https://cdn.materialdesignicons.com/3.8.95/) |
There was a problem hiding this comment.
If you check the charmap json you produced the total icons are at 3895 now, not 3039.
There was a problem hiding this comment.
I did not find the exact number for changing the number, so I prefered to let the same one. I will update this number, thanks!
There was a problem hiding this comment.
@steff456, please create a new PR to fix those minor details (here and in docs/source/usage.rst).
There was a problem hiding this comment.
I did not find the exact number for changing the number, so I prefered to let the same one. I will update this number, thanks!
The number is easy to find if your JSON is pretty-formatted (which yours was) ... simply count the lines in your charmap json and subtract 2 to account for the opening and closing curly braces. (It's one keyword per icon after all.)
|
And I see the PR is merged now, hmmm... New PR then I guess if you wanna fix it? |
|
@darkvertex, yes, please do so. |
|
@steff456 btw, just mentioning it in case you didn't see it, in the If you want to save time updating next time, it works something like this: It also renames the font title in the ttf, which is critical for the different styles to work properly... On that note, if you didn't use the custom command, did you do that when you updated the files? If not, this is probably a broken release. I'll doublecheck when I have a minute. |
I think this is already automatically tested. |
You are right, I forgot about this function: https://github.com/spyder-ide/qtawesome/blob/master/qtawesome/tests/test_qtawesome.py#L22 |
|
@darkvertex I updated FontAwesome as instructed, using the instruction you previously mentioned. I have to update the total number of icons for material design in the readme. |
Fixes #119