[#21018] - [4.0] Correct hits count icon in article info
- Fixed in Code Base
- 8 Jul 2018
- Medium
- Build: 4.0-dev
- # 21018
- Diff
- richard67:patch-1
User tests: Successful: Unsuccessful:
Pull Request for new issue.
Summary of Changes
In article info no eye icon is shown left beside the hits counter. See screenshot in section "Actual result" below.
The reason is that in file layouts/joomla/content/info_block/hits.php the wrong class fa-eye-o is used.
Correct would be fa-eye, see screenshot in section "Expected result" below.
The following screenshot of findstr commands issued in a Windows command shell shows following:
- The only PHP file in current 4.0-dev using
fa-eye-oislayouts/joomla/content/info_block/hits.php, see 1st findstr command. - In general in all other files
fa-eyeis used without anything appended to the end, see 2nd command. - Details in PHP files see 3rd command.
Testing Instructions
Reviev by a maintainer should be enough, but if not:
Use current 4.0-dev with Casiopeia template and no overrides for com_content or layouts/joomla/content.
Show article info including the hits counter in some article.
Check icon beside hits counter and inspect html.
Expected result
Actual result
Documentation Changes Required
No.
| Status | New | ⇒ | Pending |
joomla-cms-bot
- | Category | ⇒ | Layout |
| Title |
|
||||||
| Title |
|
||||||
| Title |
|
||||||
Sure, but that should be subject of another PR. This one here just corrects it consistently with other icons in current 4.0-dev.
If this here is corrected, then a later search for fa-eye for doing that future PR would be less confusing.
There is a major problem here, you are using a class of font awesome but the layout is not including the dependency (eg the css that loads the font awesome). So you re expecting that font awesome is already loaded, which bring us to:
The performance consideration: font awesome can be another big drag if it is included as normal css or part of the template (eg part of the critical path).
Obviously we need a better way to deal with the icons and this has been already discussed: joomla/40-backend-template#441 (comment)
but I'm still willing to find a way to both decouple FA and also do icons in a very performant way ootb for J4.
Sure, but that's as I said not subject of this PR. It changes nothing on using or not using font awesome, it just corrects the used class name in the same way as it is already used in other code.
So I would like to de-couple this PR here from bigger changes.
As it stands right now, this PR is correct.
I have tested this item
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/21018.
@dgrammatiko Well as I said, for current 4.0-dev code it is correct, it makes the icon be shown while currently it is not show. But you may be right that this PR is useless because things will be changed anyway. So I leave it up to George to decide.
@dgrammatiko this PR is fine as it just fixes currently existing issue. Changing the way icons are used is a different topic and out of scope of this PR.
So we will have to come back and redo this... Anyways obviously the class is wrong but would be great if instead of the span we had the actual svg there.
@dgrammatiko The screenshots of my findstr commands in the description of this PR shows that this has to be changed on other places, too. All the found usages of fa-eye in PHP files show that it is used with a span element. It does not make sense that I change this in my PR in 1 file, and other changes have to be done in a future PR, that's why I will leave to the future PR also that change for the file handled by this PR here.
instead of silly little arguments the time could have been spent getting a pr with working svg
I have tested this item
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/21018.
| Status | Pending | ⇒ | Ready to Commit |
RTC
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/21018.
wilsonge
- | Status | Ready to Commit | ⇒ | Fixed in Code Base |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2018-07-08 20:24:30 |
| Closed_By | ⇒ | wilsonge | |
| Labels |
Added:
?
|
||
Just a thought. Wouldn't it better to use generic
.icon-*classes instead of FontAwesome specific classes?