User tests: Successful: Unsuccessful:
Pull Request for Issue # .
Adds the ability to use menu image css for icons when menu image is not used.
add "icon-home" to the menu image css option.
view page. You should see icon then title.
Add explanation stating the the menu image class can also be an icon class.
Status | New | ⇒ | Pending |
Category | ⇒ | Modules Front End |
@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.....
@brianteeman what version is that? When I use it it sets the width to icon width only...
and the appropriate css
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.
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
Or you could use the same css that all the joomla.org web sites use for the joomla logo
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?
you also modified css is that correct?
Yes of course
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?
I am not going to enter into a debate on is CSS code or not.
This pr is simply wrong
well, that's your opinion and your certainly entitled to it. I'm going to ask for a review by others.
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.
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.
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.
@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.
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
Status | Pending | ⇒ | Closed |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2019-09-20 23:25:42 |
Closed_By | ⇒ | N6REJ | |
Labels |
Added:
?
|
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=
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
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/
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
