Upgrade: Font Awesome 4.2.0 - Take #2 - #6055
Conversation
1080b91 to
42bffd4
Compare
|
@clrux, looks like the HTML class name changes caused some UI-related automated tests to fail (see - https://jenkins.testeng.edx.org/job/edx-all-tests-auto-pr/18673/). I won't have time to look through this until perhaps later today or this week, if its not too confusing or muddy water, feel free to try your hand or you can wait for me to try to tackle things. |
b92be3a to
f81d577
Compare
8bc7055 to
ed99e9e
Compare
ed99e9e to
2417173
Compare
|
@andy-armstrong, I've just freshly rebased this branch/PR. Tests are running and there should still be one failure around the staff lock on the unit/container view in Studio. Mind taking a look once the results are in? |
507c8f2 to
f602e9a
Compare
|
@marcotuts, can you review the FED changes here? Also, @marcotuts and @frrrances, I'm getting a sandbox up and running shortly, would you mind proofing both Studio and LMS (navigating to most views) to make sure icons are rendering as expected? @feanil, heads up on this vendor asset upgrade. I don't think there's anything greatly changing for prod environments, but if you smell anything funky let me know. FYI, @clrux. |
There was a problem hiding this comment.
is "fa" a separate class here or is this a duplication typo? (double-checking.)
edit: I'm seeing 'fa' as a standalone class below. Is this serving the same role as the old 'icon' class?
There was a problem hiding this comment.
@marcotuts fa is the Font Awesome class prefix. In this case, there's a duplicate. Good catch.
There was a problem hiding this comment.
actually it looks like @talbs is leaving the bare "fa" probably so we can target these icons specifically in case we need to...
There was a problem hiding this comment.
The new syntax requires two classes fa and fa-[icon-name] for the Font Awesome vendor styling to be rendered properly. I'm adding both for vendor support, but I'm leaving a class of icon in as well for our own app/edX-styling that's not font icon library dependent (think about if we switch down the road or something). Hope that clears things up.
There was a problem hiding this comment.
same note around bourbon's transform mixin applies here if we decide to use it.
|
👍 - a quick run through on a sandbox would be good but otherwise this looks good to go! (proofing primarily for wiki styles and the icon-stack stuff) |
|
@marcotuts, thanks for the review. I'll definitely address all of the good feedback and catches you noted. In the meantime a sandbox is up: Please have at it, @frrrances and @marcotuts, when you can to review rendered UI. I'll do the same. |
876fa9f to
e2762ed
Compare
|
@talbs - couldn't find anything on a review of the sandbox! The couple items I stumbled across wehre also present on edx.org and don't need to impact this PR. (bugs filed for those) |
|
👍 from an ops perspective. I don't think this will have any issues if it's already working correctly on sandboxes. |
e2762ed to
55a9a28
Compare
* updating vendor files * updating class syntax (to new FA-based classes) for all UI elements * correcting broken tests
55a9a28 to
c5d2dd7
Compare
|
Rebased from master and freshly pushed. Once tests pass, I'll try to merge this fella. |
Upgrade: Font Awesome 4.2.0 - Take #2
5 participants
This work updates our Font Awesome vendor files to the most recent version. This update required an expansive change in semantics and icon name references. A few notes:
.iconto any font awesome reference for our own additional styling/tweaking.