Skip to content

[4.0] Correct hits count icon in article info#21018

Merged
wilsonge merged 1 commit intojoomla:4.0-devfrom
richard67:patch-1
Jul 8, 2018
Merged

[4.0] Correct hits count icon in article info#21018
wilsonge merged 1 commit intojoomla:4.0-devfrom
richard67:patch-1

Conversation

@richard67
Copy link
Member

@richard67 richard67 commented Jul 8, 2018

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-o is layouts/joomla/content/info_block/hits.php, see 1st findstr command.
  • In general in all other files fa-eye is used without anything appended to the end, see 2nd command.
  • Details in PHP files see 3rd command.

finstr-fa-eye

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

hits-icon-present

Actual result

hits-icon-missing

Documentation Changes Required

No.

@richard67 richard67 changed the title Correct class fa-eye-o to fa-eye for hits count [4.0] Correct class fa-eye-o to fa-eye for hits count Jul 8, 2018
@richard67 richard67 changed the title [4.0] Correct class fa-eye-o to fa-eye for hits count [4.0] Correct class fa-eye-o to fa-eye for hits count in article info Jul 8, 2018
@richard67 richard67 changed the title [4.0] Correct class fa-eye-o to fa-eye for hits count in article info [4.0] Correct hits count icon in article info Jul 8, 2018
@SharkyKZ
Copy link
Contributor

SharkyKZ commented Jul 8, 2018

Just a thought. Wouldn't it better to use generic .icon-* classes instead of FontAwesome specific classes?

@richard67
Copy link
Member Author

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.

@dgrammatiko
Copy link
Contributor

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.

@richard67
Copy link
Member Author

richard67 commented Jul 8, 2018

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.

@SharkyKZ
Copy link
Contributor

SharkyKZ commented Jul 8, 2018

As it stands right now, this PR is correct.

@SharkyKZ
Copy link
Contributor

SharkyKZ commented Jul 8, 2018

I have tested this item ✅ successfully on cf5a6c6


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

@dgrammatiko
Copy link
Contributor

@SharkyKZ did you event read the comment of @wilsonge I posted above?
The decision is to use svg icons in the layouts and not the font, so no the PR is not correct!!!

@richard67
Copy link
Member Author

@wilsonge Please review this PR here and close it if necessary.

@richard67
Copy link
Member Author

@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.

@SharkyKZ
Copy link
Contributor

SharkyKZ commented Jul 8, 2018

@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.

@dgrammatiko
Copy link
Contributor

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.

@richard67
Copy link
Member Author

richard67 commented Jul 8, 2018

@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.

@brianteeman
Copy link
Contributor

instead of silly little arguments the time could have been spent getting a pr with working svg

@Quy
Copy link
Contributor

Quy commented Jul 8, 2018

I have tested this item ✅ successfully on cf5a6c6


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

@Quy
Copy link
Contributor

Quy commented Jul 8, 2018

RTC


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

@joomla-cms-bot joomla-cms-bot added RTC This Pull Request is Ready To Commit and removed PR-4.0-dev labels Jul 8, 2018
@wilsonge wilsonge merged commit 3e06761 into joomla:4.0-dev Jul 8, 2018
@joomla-cms-bot joomla-cms-bot added PR-4.0-dev and removed RTC This Pull Request is Ready To Commit labels Jul 8, 2018
@wilsonge wilsonge added this to the Joomla 4.0 milestone Jul 8, 2018
@richard67 richard67 deleted the patch-1 branch July 8, 2018 20:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants