Skip to content

Comments

[4.0] Switch rating stars from png images to icons#22258

Closed
htmgarcia wants to merge 9 commits intojoomla:4.0-devfrom
htmgarcia:voting-stars
Closed

[4.0] Switch rating stars from png images to icons#22258
htmgarcia wants to merge 9 commits intojoomla:4.0-devfrom
htmgarcia:voting-stars

Conversation

@htmgarcia
Copy link
Contributor

Pull Request for Issue NA .

Summary of Changes

The Content - Vote plugin currently uses png images to display the rating stars in articles. Let's take advantage of Font Awesome and use icons instead.
lorem_ipsum_rocks___3_

Testing Instructions

Enable Content - Vote plugin for articles
Preview in frontend

Expected result

Actual result

Documentation Changes Required

@joomla-cms-bot joomla-cms-bot changed the title Switch rating stars from png images to icons [4.0] Switch rating stars from png images to icons Sep 19, 2018
@brianteeman
Copy link
Contributor

This adds a hard requirement to use font awesome in all front end templates. Is that really desirable?

@mbabker
Copy link
Contributor

mbabker commented Sep 19, 2018

It's an overridable layout (the whole voting capability should still just be inlined into com_content but ¯\_(ツ)_/¯)

@htmgarcia
Copy link
Contributor Author

@brianteeman
Copy link
Contributor

it may well be right now but one of the aims was to remove that requirement not add to it

@htmgarcia
Copy link
Contributor Author

@brianteeman this "let's not stick with Bootstrap" but at the same time "let's use it partially" approach is inconsistent. I find impossible to know where is fine and where is not to make use of dependencies like BS4 (and now same case with Font Awesome).


A clear guidance on this matter would be useful.

@mbabker
Copy link
Contributor

mbabker commented Sep 19, 2018

My two cents. We're never going to have a core UI that is 100% decoupled from any framework. So, as long as we have all this "framework coupling" in overridable layouts, at this point I'm leaning toward "who cares, replace it if you want".

@brianteeman
Copy link
Contributor

I am just repeating what "powers that be" have said about frontend decoupling - not giving an opinion as thatis too dangerous

@htmgarcia
Copy link
Contributor Author

Keeping consistency is a good practice, and also is my personal opinion.

Regards

@htmgarcia htmgarcia closed this Sep 19, 2018
@htmgarcia htmgarcia deleted the voting-stars branch September 19, 2018 20:34
@laoneo
Copy link
Member

laoneo commented Sep 20, 2018

I would rather go with joomla/40-backend-template#441. I'v used a similar technique in DPCalendar. Like that it is possible for a template to override icons. If there is interest, I can migrate the pr here.

@brianteeman
Copy link
Contributor

@laoneo is that needed? This is a front-end layout that can already be overridden

@laoneo
Copy link
Member

laoneo commented Sep 20, 2018

Icons have always been a pain in Joomla. There should be an easy way to change them and to have a strategy which serves the core and extension devs.

Making always an override of every view when you want to change the icon set sounds for me not optimal. Keep in mind we have forms, etc. with icons. Having a common layout where you can pass a string and it returns an icon is for me the way to go.

@mbabker
Copy link
Contributor

mbabker commented Sep 20, 2018

Having a common layout where you can pass a string and it returns an icon is for me the way to go.

Explain this bit in more detail. Because right now I'm afraid you're going to have this as a layout file, which would be about as crappy as that endtab.php file:

<?php
$icon = $displayData['icon'];
?>
<span class="<?php echo $icon; ?>"></span>

@laoneo
Copy link
Member

laoneo commented Sep 20, 2018

We do load them inline in a JLayout. So the template dev can override the layout itself or place the required icons in the template folder itself.

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.

6 participants