Skip to content
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

[4.0] add fa Regular icons back into core availability #31096

Merged
merged 8 commits into from Oct 27, 2020
Merged

[4.0] add fa Regular icons back into core availability #31096

merged 8 commits into from Oct 27, 2020

Conversation

N6REJ
Copy link
Contributor

@N6REJ N6REJ commented Oct 15, 2020

Pull Request for Issue # .

Summary of Changes

adds back the ability to use fontawesome regular fonts into core.

Testing Instructions

go to /administrator/index.php
inspect eye.
do the same for front end login
apply pr
using the inspector, change "fas fa-eye fa-fw" to "far fa-eye fa-fw"
notice the icon changed.

Actual result BEFORE applying this Pull Request

image

Expected result AFTER applying this Pull Request

image

Documentation Changes Required

Our implementation of "Fontawesome Regular icons" is a limited 151 icon set from the 1852 icons included in their "pro" version. This is by design from FA. They don't mention that ANY regular icons are included in the free set at all on their site. Only the icons listed here are included.

@joomla-cms-bot joomla-cms-bot added NPM Resource Changed This Pull Request can't be tested by Patchtester PR-4.0-dev labels Oct 15, 2020
@adj9
Copy link

adj9 commented Oct 15, 2020

I have tested this item ✅ successfully on 8a17b03

There is the modify in HTML.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/31096.

@richard67
Copy link
Member

@adj9 How can you mark a PR with a successful test when there is a discussion on it about an obvious mistake in it?

Maybe the discussion is not visible in the issue tracker because it was a code review comment. But please always check a PR on GitHub, too, not only in the issue tracker, because only on GitHub you can see which files were changed and if there are unresolved review comments.

Thanks in advance.

@richard67
Copy link
Member

@adj9 Please repeat your test. Thanks in advance.

@ceford
Copy link
Contributor

ceford commented Oct 16, 2020

I tested this but did not see any change in the source code and the diff only shows changes to files processed by npm.

@richard67
Copy link
Member

@ceford As far as I understood the testing instructions, you will only see a difference if you change the code in the html markup from "fas fa-eye fa-fw" to "far fa-eye fa-fw" e.g. by manipulating the markup in your browser's developer tools or by editing the PHP file. The icon should then visibly change it's look from solid to regular.

The changes of this PR here do not change the html markup, they only change the scss and so the compiled css so that the icon still is shown when far is used instead of fas.

@N6REJ Correct me if my understanding is wrong.

@ceford
Copy link
Contributor

ceford commented Oct 16, 2020

Baffled again! I now see the eye icon change appearance in atum. But it does not make any difference whether or not I have the patch applied. This last part of the dif looks odd:

 // Backend Template stuff
 @import "../../../administrator/templates/atum/scss/mixin";
diff --git a/templates/cassiopeia/scss/template.scss b/templates/cassiopeia/scss/template.scss
index 896fb521bf09..1544b8a27a1f 100644
--- a/templates/cassiopeia/scss/template.scss
+++ b/templates/cassiopeia/scss/template.scss
@@ -14,8 +14,9 @@
 
 // "Font Awesome 5 Free"
 @import "../../../media/vendor/fontawesome-free/scss/fontawesome";
-@import "../../../media/vendor/fontawesome-free/scss/solid";
 @import "../../../media/vendor/fontawesome-free/scss/brands";
+@import "../../../media/vendor/fontawesome-free/scss/regular";
+@import "../../../media/vendor/fontawesome-free/scss/solid";
 

There is no mention of import into atum.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/31096.

@N6REJ
Copy link
Contributor Author

N6REJ commented Oct 16, 2020

@richard67 100% on the money
@ceford for some reason it was never removed in atum ( which turned out to be a good thing )

@import "../../../../../../media/vendor/fontawesome-free/scss/regular";

the diff probably looks odd to you because I put the imports in alphabetical order.
which brings up that atum was NOT in proper order at all. So thanks for that.

@particthistle
Copy link
Member

I have tested this item ✅ successfully on fb5cc3e

Swapping fas for far when inspecting code on the administrator area after applying the patch changed the icon from solid to regular as expected as the result.

Handy to know Font Awesome's Regular free icons are now included in the backend.

@N6REJ can you replicate this PR over into Cassiopeia, as seeing if it carried over to the front end didn't repeat the backend behaviour.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/31096.

@N6REJ
Copy link
Contributor Author

N6REJ commented Oct 17, 2020

@particthistle not sure how to do that but I can try.

@particthistle
Copy link
Member

particthistle commented Oct 17, 2020

@particthistle not sure how to do that but I can try.

Looking at it more this afternoon it might be a little more difficult as the fonts are currently in the atum template, and not a library loading on it's own.

As mentioned though in #30793 once you know where the files are you could reference them when building a template.

@degobbis
Copy link
Contributor

degobbis commented Oct 17, 2020

I have tested this item 🔴 unsuccessfully on fb5cc3e

Tested unsuccessfully with 4.0-dev after run npm run build:css
The icon far fa-eye fa-fw looks equal before and after the patch.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/31096.

@particthistle
Copy link
Member

@degobbis The PR doesn't actually change the class to far fa-eye fa-fw, you need to do that with a manual code inspection.

Here's what the login page fa-eye looks like after my test...
31096

@degobbis
Copy link
Contributor

@particthistle This is exactly what I did, before and after the patch.
The icon far fa-eye fa-fw looks the same before and after the patch.

@N6REJ
Copy link
Contributor Author

N6REJ commented Oct 17, 2020

@particthistle This is exactly what I did, before and after the patch.
The icon far fa-eye fa-fw looks the same before and after the patch.

it's a VERY subtle change. if you want something more dramatic a shift use envelope.. https://fontawesome.com/icons?d=gallery&q=envelope&m=free
so, 'far fa-envelope fa-fw" & "fas fa-envelope fa-fw"

@jwaisner
Copy link
Member

I have tested this item ✅ successfully on fb5cc3e

Confirmed after patch that changing to "far fa-eye fa-fw" on eye image results in the new eye icon. This was confirmed on front end, backend, and installation.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/31096.

@jwaisner
Copy link
Member

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/31096.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Oct 23, 2020
@rdeutz rdeutz merged commit 2c8e054 into joomla:4.0-dev Oct 27, 2020
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Oct 27, 2020
@rdeutz rdeutz added this to the Joomla 4.0 milestone Oct 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NPM Resource Changed This Pull Request can't be tested by Patchtester
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet