? Pending

User tests: Successful: Unsuccessful:

avatar N6REJ
N6REJ
20 Sep 2019

Pull Request for Issue # .

Summary of Changes

Adds the ability to use menu image css for icons when menu image is not used.

Testing Instructions

add "icon-home" to the menu image css option.
view page. You should see icon then title.
image

Expected result

image

Actual result

image

Documentation Changes Required

Add explanation stating the the menu image class can also be an icon class.

avatar N6REJ N6REJ - open - 20 Sep 2019
avatar N6REJ N6REJ - change - 20 Sep 2019
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 20 Sep 2019
Category Modules Front End
avatar N6REJ N6REJ - change - 20 Sep 2019
The description was changed
avatar N6REJ N6REJ - edited - 20 Sep 2019
avatar brianteeman
brianteeman - comment - 20 Sep 2019

Sorry this is not correct. That field is for adding a class to the image (if added)

To use css to display a font-icon for example you should be using the link-class field above and the appropriate css

See example below with no code change at all
menu

avatar N6REJ
N6REJ - comment - 20 Sep 2019

@brianteeman now your making me remember why that was a "problem" in my mind :P lol
off to go search the gray matter for my reasoning.....

avatar N6REJ
N6REJ - comment - 20 Sep 2019

@brianteeman what version is that? When I use it it sets the width to icon width only...
image

avatar brianteeman
brianteeman - comment - 20 Sep 2019

and the appropriate css

avatar N6REJ
N6REJ - comment - 20 Sep 2019

so, your saying we need to add css to make it function? Seems silly. The purpose of the 'link css' was to add the ability to put additional classes onto a menu link. Now, I'll admit THEORETICALLY there is no reason it shouldn't work for icons, but clearly there is an issue.
If you can explain more about your settings in how they differ from current staging then perhaps I can agree with your results.

avatar brianteeman
brianteeman - comment - 20 Sep 2019

The purpose of the 'link css' was to add the ability to put additional classes onto a menu link.

Exactly - and thats what you do when you use a font icon

If you can explain more about your settings
It was something like

[class^="nav icon-"]:before,
[class*=" nav icon-"]:before {
	display: inline-block;
	width: 14px;
	height: 14px;
	margin-right: .25em;
	line-height: 14px;
}

PS the code you wrote is not consistent with Joomla code style or convention either

avatar brianteeman
brianteeman - comment - 20 Sep 2019

Or you could use the same css that all the joomla.org web sites use for the joomla logo

avatar N6REJ
N6REJ - comment - 20 Sep 2019

Or you could use the same css that all the joomla.org web sites use for the joomla logo

I've not touched anything about J! other then add the icon ability so its not 'ME'. This is staging so...
So, in order to achieve the look you did it was NOT as simple as putting in the icon class name you also modified css is that correct?

avatar brianteeman
brianteeman - comment - 20 Sep 2019

you also modified css is that correct?

Yes of course

avatar N6REJ
N6REJ - comment - 20 Sep 2019

See example below with no code change at all

so thats not an entirely correct statement then. What is the harm in having an ICON class associate with an image class? Especially considering it works w/o ANY modification?

avatar brianteeman
brianteeman - comment - 20 Sep 2019

I am not going to enter into a debate on is CSS code or not.
This pr is simply wrong

avatar N6REJ
N6REJ - comment - 20 Sep 2019

well, that's your opinion and your certainly entitled to it. I'm going to ask for a review by others.

avatar mbabker
mbabker - comment - 20 Sep 2019

This is a B/C break for however that field is used. If you want to override the module in your template to do this then more power to you but as Brian said this PR is not right for core.

avatar N6REJ
N6REJ - comment - 20 Sep 2019

@mbabker why is it a b/c break? it's an enhancement not change? images & image class still behave the same as always.
just asking....

avatar mbabker
mbabker - comment - 20 Sep 2019

It changes the use of the field in a way not compatible with its previous use. The field is intended as a CSS class for the image, it is not intended to add additional markup to the menu item in the case an image is not used (i.e. a font icon), and we'll ignore the semantically incorrect markup for the moment because that's a minor detail that doesn't matter until someone decides this "enhancement" is OK to accept in principle. In the case there is something in that field and an image is not set, it will alter the display of menu items. Assuming anything I have been involved with creating or revising has any value to this project anymore, this breaches the HTML B/C specified in the development strategy.

avatar brianteeman
brianteeman - comment - 20 Sep 2019

You would also need to change the label and the tooltip and the associated documentation (if it exists) to explain this enhancement otherwise no one will know about it.

If you want to use a font icon then you add a link class. By the very definition of the term link class that would be the expected nature of the field.

If you want to use an image then you use the image field and if you want to style that image you use the image class field. y the very definition of the term image class that would be the expected nature of the field.

avatar N6REJ
N6REJ - comment - 20 Sep 2019

@brianteeman I understand about using the link & class.
@mbabker Ty for the detailed explanation Michael. Thats what I was looking for. In my, I guess wrong, thinking I see the icon as an image not as a link. That's where this stemmed from.

semantically incorrect markup
for my own edification what is wrong with the markup?

I appreciate both of your explanations.

avatar N6REJ
N6REJ - comment - 20 Sep 2019

After discussing this more, I'm going to close the pr. If anyone does decide to use it it'll be in the archives :D

avatar N6REJ N6REJ - change - 20 Sep 2019
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2019-09-20 23:25:42
Closed_By N6REJ
Labels Added: ?
avatar N6REJ N6REJ - close - 20 Sep 2019
avatar brianteeman
brianteeman - comment - 20 Sep 2019

semantically incorrect markup
for my own edification what is wrong with the markup?

In your markup you used <i which is abused elsewhere a being for <icon
If you look in core we never use that and instead use <span class=

avatar N6REJ
N6REJ - comment - 20 Sep 2019

Thats an argument I've heard so much of its hard to know who's right. But of course as far as the project is concerned it's standards/rules trump all

avatar N6REJ
N6REJ - comment - 20 Sep 2019

And here you go, straight from the horses mouth :P

Font Awesome is designed to be used with inline elements and we recommend sticking with a consistent HTML element to reference them by in your project. We like the <i> tag for brevity and for the fact that most folks are using <em></em> for emphasized/italicized semantic text these days. If that’s not your cup of tea, using a <span> is more semantically correct.
https://fontawesome.com/how-to-use/on-the-web/setup/getting-started

Except, here you have google stating just the opposite LOL
https://google.github.io/material-design-icons/

Add a Comment

Login with GitHub to post a comment