@N6REJ any spezial thing / szenario that need te be tested?

just check spacing in element inspector as shown in these 2 images. The difference is much more dramatic when using a non-core template such as joostrap base.
BEFORE:

AFTER:

I wpuld just do the opposite, i.e. normalise to use   before and after the icon everywhere.
Before and after is necessary to cope with RTL
Before your patch:

After your patch

I rejected that because it would be inconsistent with how link css style
behaves. It's much easier to add a space in the language .ini then to
remove a space via .css. For example in the "new" icon, you could simply
change the language.ini from "New" to " New ".
That would require the addition of a language element for "print",
"email" , "edit" but it could be done.
for RTL adding a :after or :before ( i'm not sure which with RTL ) then
Margin-right: .3em;
would accomplish close to the same thing as adding a space, and would
even allow for additional space should it be desired. Hard coding data
( text strings, characters, etc, ) into code is normally a bad idea. I
think it is in this case also. Infact The very fact that print, edit,
email are hardcoded is wrong in my opinion, they should be language strings.
Bear
On 9/28/2014 02:12, infograf768 wrote:

I wpuld just do the opposite, i.e. normalise to use | | before
and after the icon everywhere.
Before and after is necessary to cope with RTL

—
Reply to this email directly or view it on GitHub
#4372 (comment).

No virus found in this message.
Checked by AVG - www.avg.com http://www.avg.com
Version: 2014.0.4765 / Virus Database: 4025/8286 - Release Date: 09/27/14

The very fact that print, edit, email are hardcoded is wrong in my opinion, they should be language strings.

Where do you see these hardcoded?

Agree on the principle that CSS is better than hardcoded stuff. In this case though I think it's overkill.

dang it, I can't find it now, of course, sigh.. I've got to look like a an idiot @ least once during a issue....
ok, so forget about the hardcoding,... lets just focus on the spacing.

Apparently your against this patch JM? why not put the space in the .ini? seems much more consistent and is a win-win as those that don't want a space ( though why they would is beyond me ) and those that do, both get what they want easily.

Anyway, lets just get a consensus of how it SHOULD be and then I can make it that way.

In my opinion, presentation, which includes spacing, should never be hardcoded in generated HTML and should always be done with CSS.

Thats how I feel too Matt, if you look @ how it effects joostrap its very obvious the existing method has issues. I guess my strongest complaint(?) is that it's much harder to remove a "space" with .css then it is to "add" the appearnce of a space

Relying on a leading and trailing space is very difficult thing to do in a
language file.

On 29 September 2014 18:34, Bear notifications@github.com wrote:

Thats how I feel too Matt, if you look @ how it effects joostrap its very
obvious the existing method has issues. I guess my strongest complaint(?)
is that it's much harder to remove a "space" with .css then it is to "add"
the appearnce of a space

—
Reply to this email directly or view it on GitHub
#4372 (comment).

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/

so what is your suggestion Brian?

I agree with @betweenbrain about using CSS. That CSS code has to be added to the PR (including the RTL part).

Hardcoding spaces in languages is the same than hardcoding spaces in markup.

@phproberto what markup are you suggesting? As it stands no markup is needed if we want the default to be the icon/text side by side with no padding. Else .3em seems to work decently.

In my opinion, presentation, which includes spacing, should never be hardcoded in generated HTML and should always be done with CSS.

% This.

I'd suggest adding letterspace to the pseudo element directly, as this is what's typically expected. The only place where this could cause a very minor issue is in the case of an icon within a button element without following text, like the article print/email dropdown button where the additional space would be unwanted. But I'd suggest this is an edge case and should be dealt with as such, removing the added space on that element precisely. FWIW, one letterspace is .25em.

I'd suggest letterspacing the entire icon class psuedo like this:

[class^="icon-"]:before,
[class*=" icon-"]:before {
    margin-right: .25em;
}

.rtl [class^="icon-"]:before,
.rtl [class*=" icon-"]:before {
    margin-left: .25em;
}

This ensure you only have a space around and icon, and you always have a space around an icon.

@nternetinspired seth I was only able to get

li[class*='-icon'] span[class*='icon-']:before {
     padding-right: .25em;
}

to work properly. if I didn't use the li preface then author icons were effected also ( which is maybe whats wanted ), but they already have spacing. so I'm assuming

.rtl li[class*='-icon'] span[class*='icon-']:before {
     padding-left:: .25em;
} 

would also be correct?

After applying @N6REJ's patch top remove the hardcoded spacing, adding the following CSS to /templates/protostar/css/template.css did the trick for me:

[class^="icon-"],
[class*=" icon-"] {
    margin-right: .25em;
}

.rtl [class^="icon-"],
.rtl [class*=" icon-"] {
    margin-left: .25em;
}

@betweenbrain it didn't adversley effect the icons like calendar and the little "eye" in author area?

Screenshots attached

@N6REJ it seems to be okay for me.

Here's what I'm seeing mat, idk if you can see the extra space being added to the authoring icons.
BEFORE

AFTER:

ok, I figured it out I think. Please let me know what you think. By using

li span[class^="icon-"], li span[class*=" icon-"] {
margin-right: .25em;
}

I wind up with
with my change BEFORE:

with my change AFTER:

if I didn't use the li preface then author icons were effected also ( which is maybe whats wanted ), but they already have spacing. so I'm assuming would also be correct?

No, that's not correct. You're over-qualifying the selector, making it too specific. The css I posted will target every element that contains an icon class, which is what we want. You see too much space in some cases because those items also have hardcoded spaces, when they shouldn't.

With the change I proposed all hardcoded spaces can be removed and replaced with this single css solution.

Ugh. Yeah, just tested this and because Icomoon applies the base font styles to the parent element and not the :beforen (where the icon is applied), the margin needs to target the parent element instead, exactly as @betweenbrain put;

[class^="icon-"],
[class*=" icon-"] {
    margin-right: .25em;
}

.rtl [class^="icon-"],
.rtl [class*=" icon-"] {
    margin-left: .25em;
}

@nternetinspired I'm sorry, but I disagree, I removed all of the ' ' . and as I showed before, the article icons still get the extra spacing. such as "icon-eye-open" and "icon-cog". This is why I needed to qualify it more.

Yes, because they have hardcoded space in html. They shouldn't.

@nternetinspired I"ll keep searching, I found icon-cogs but not icon-eye-open and that group.. so far...
I'm really really frustrated now. I can't find the calendar one anywhere sigh
I don't see a " " anywhere here.

            <?php if ($params->get('show_publish_date')) : ?>
                <dd class="published">
                    <span class="icon-calendar"></span>
                    <time datetime="<?php echo JHtml::_('date', $this->item->publish_up, 'c'); ?>"itemprop="datePublished">
                        <?php echo JText::sprintf('COM_CONTENT_PUBLISHED_DATE_ON', JHtml::_('date', $this->item->publish_up, JText::_('DATE_FORMAT_LC3'))); ?>
                    </time>

Guys, I need help. There is 4px coming from somewhere. I suspect its because the fonts are 16px fonts and yet these are 13px. But I'll be danged if I can find it. I've spent over 20 hrs trying to. Can someone please help me find the extra space? It's only related to the icomoon icons.

@N6REJ Where are you seeing the extra 4px?

if you look @ the calendar icon, the open eye, you'll see that there is a slight white space. Changing the width of calendar to 9px instead of 13 removes this space . the default icon width is 16px so I suspect the diff is the 16-13 but I'll be dang if I can prove it. if you put a F infront of the published: you'll easily see the space. This is exactly why i used the more structured .css earlier.

Right, yes. I thought you literally meant 4px. This is a whitespace in the html, because span elements are treated like words in a sentence, any html whitespace is respected, unless the font-size is 0. As with hardcoded spaces in html or lang files, relying on html whitespace to give icons spacing is pretty unreliable (if a users minimises their html, boom. No icon space.)

This is with your patch applied to icons.php and no additional css. Hits icon:

Print icon:

I guess we should re-think how the print and email icons are included, in icon.php, as removing formatting (and therefore whitespace) in layouts and views isn't a viable option…

Maybe the dropdown icon markup should be moved out into the layout instead of the helper? Anyone know why they are in the helper currently?

Maybe the dropdown icon markup should be moved out into the layout instead of the helper?

That's definitely worth looking at.

@nternetinspired yes, my patch will require a .css styling ( or 2 ) be applied, in actuality there already is one...

    *margin-right: .3em;

I dont' understand the * and quite frankly it doesn't seem to work anyway.
What would you like me to do @ this point? If I apply both stylings it will work as currently built in helper

@N6REJ I'm concerned that your proposed css isn't scoped precisely, so it has the potential to cause an issue elsewhere, i.e. where an icon span follows a li that does have html whitespace. Then we'd have double spacing.

Just to be clear though, removal of the hardcoded spaces is 100% the correct thing to do, IMO. Thanks for bringing it to the fore :)

I've found whitespace for icons all over the place and while I've gotten them removed, its not a solution as long as we still have the 3px spacing for the authoring info icons. I've searched high and low and can't figure out where its coming from. Can anyone please assist in finding it?

There is no 3px spacing. That is letterspace and, as mentioned in #4372 (comment), it's coming from html whitespace. There's nothing you can realistically do about that because removing it would require removing all indentation and nesting in the php.

I'm sorry I dont' understand what you mean about its from HTML spacing. If I move the exact same html coding OUTSIDE of the TIME declaration the 3px spacing is NOT there. Yet there is no place I see that creates it. Adding time{margin-left: -3px;} removes it. Same for eye with META. There is no whitespace inside the span's its simplly span /span
so, if we can't track this down, then authoring icons will have 6px spacing while everything else has 3px. IF thats ok, then fine, I'll submit everything after I quadruple check and be done with it.

I'm not talking about an html whitespace character, but about an inherent property of html to treat tabs, spaces, newlines as whitespace. The nesting, and indentation, of elements in php therefore introduces whitespace, which is what can be seen the the first image I posted above. There's a new line between the icon span and the meta, and that's your space.

The only sensible way to remove that is to remove the indentation from the PHP, minifying, it so it all appears on a single line. That's not going to be popular of course. The other reliable method is to set the font-size of the parent element to 0/0 (because then the letterspace = 0) and then reset the correct font-size on the child element, but I honestly don't think it's worth it.

I think the best option is to tackle the outlier, which is the print / email icons. Almost every other icon instance will include the inherent html letterspace, so I think it's a case of learning to live with it as realistically it's impractical to tackle.

@nternetinspired so let me c if I get this right. i'm fairly sure I tried putting everyone on one line but yes, like you said, @George Wilson would kill me for messing with code styling. The issue is an inherent kerning issue built into html fonts ( which I don't understand cause the print/email icons don't do the same thing. But be that as it may, your proposing to leave the extra 3px spacing, add the 3.25 px spacing from the margin-right: .25em; , make sure all the true SPACES" and &#160's that effect icons are removed, and call it good?

@N6REJ What you write seems reasonable to me.

@N6REJ Yes, it's a property of html, though nothing to do with kerning. Print / mail icons don't have any whitespace in them because the markup is not being rendered in a view.

Yes, I don't believe we should try and hack around an inherent feature of html in this case.

No, I don't think we should be adding additional CSS space, other than to correct for the email/print icons, as a short term measure. Ultimately, it would be better if the markup for those was also in a layout.

Yes, I believe all hardcoded whitespace characters present only for spacing purposes should be removed wherever possible.

Yes, it's a property of html, though nothing to do with kerning. Print / mail icons don't have any >whitespace in them because the markup is not being rendered in a view.
HUH? How you figure its not in a view? You are very confusing

@N6REJ can you try this PR #3413 with your code and see if the whitespace in email/print icons is fixed?

@dg its not exactly pertinent because your PR only functions if gzip is on. This is present irregardless of gzip

@N6REJ I know its not exactly relevant but as people are talking about whitespace in HTML’s stracture, I thought it might help if you try to eliminate that. And as far as I remember, (the PR is quite old) this is what that PR does. If it works on gzip then maybe we can make a switch to make it independent like on/off. But regardless of that PR you can try to minify the output here http://www.willpeavy.com/minifier/ and see if that corrects the spacing.

ok, its all working properly now. I hope this "fix" satisfy s everyone

Thanks @N6REJ. I can tell that you put a lot of work into this. At a quick glance, it looks good, but will need a careful review as so many files have been touched. Let's get this tested and reviewed.

@betweenbrain Thanks Matt, I tried to be careful so hopefully nobody will find any big issues.

@N6REJ
This needs css and rtl css imho for Isis and hathor
This needs rtl css in protostar (therefore in bootstrap_rtl.less.
Also changes to .css have to be first done in the .less file concerned , then generated by CLI generatecss.php

Yes, it's a property of html, though nothing to do with kerning. Print / mail icons don't have any >whitespace in them because the markup is not being rendered in a view.
HUH? How you figure its not in a view? You are very confusing

Markup for those icons is created in the functions in https://github.com/joomla/joomla-cms/blob/staging/components/com_content/helpers/icon.php . They aren't in a view file and so there's no formatting to create the whitespace that is seen elsewhere.

@dg its not exactly pertinent because your PR only functions if gzip is on. This is present irregardless of gzip

Actually, this is relevant because, as I've already pointed out, there is space after icons that comes from purely from the whitespace in the html. Minifying html will affect the spacing of icons.

@nternetinspired regarding view.. ok, I get what you mean now. That explains ALOT.
regarding caret, imho yes, because again, cog and caret are data and subject to change, the spacing would not be. Perhaps they want it smaller then normal? With the spacing its more work ( yes i know it's negligible ) and because it was removed for and aft its "even" so no lopsidedness introduced.
regarding ie7, tyvm I was wondering what it was doing because it certainly didn't work that I saw.
I'll remove it.
regarding the other PR, are you suggesting that I use his pr instead? My regex sucks so if we're going to do things that way I would prefer that someone else more knowledgeable do it, and I'll just abandon my work here.

@infograf768 i see the class's

/* From Bootstrap */
[class^="icon-"],
[class*=" icon-"] {
    display: inline-block;
    width: 14px;
    height: 14px;
    .ie7-restore-right-whitespace();
    line-height: 14px;
}

only in icomoon.less am I mistaken that .rtl will pull it from there as well? I don't see that same class in bootstrap-rtl.less. Please advise.
regarding admin: How do you feel about doing isis and hathor separate of this PR since that will involve alot more files? The checking that I did do showed alot more consistency in admin then there was in protostar.
It didn't dawn on me to check for the declaration's in .less, I'll do that ty.

@all how does "\media\jui\less\sprites.less" come into play? We seem to have similar declarations in 2 places, sprites and icomoons.
Thanks for the guidance to polish it guys, its amazing how something so seemingly simple turns into a hydra LOL

You would have to override only the margin in bootstrap_rtl.less

+[class^="icon-"],
+[class*=" icon-"] {
+   margin-left: .25em;
+}

and I guess add also add something like (did not test that one)

+dd > span[class^="icon-"] + time,
+dd > span[class*=" icon-"] + time{
+   margin-right: -.25em;
+}

@infograf768 i understand what you mean. I'll try to wrap this whole thing up tomorrow. Today is a screwy day.

ok, its ready for testing now. Everything moved out to .less compiled and tested. Looks great.

@test
This is image before apply patch

After apply patch

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

I believe you have your images backwards... the bottom one should be "before" Can you double check please.

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

@N6REJ I'm fairly sure the second one is the after image as that article has 8 hits in the screenshot, but only 6 in the first.

It looks good to me.

@suredweb idk what is going on, I've fixed that silly file 4x now. Please retest. Sorry for the obvious failure before. When I checked I could swear it was fixed.

@N6REJ I downloaded you branch and tried to rebase it manually but I think you started from an outdated branch or maybe the wrong branch (master?). I suggest you that you start from scratch on a new branch (created from a new clean and updated staging) just copying the files you really want to modify.

Also create huge lines is not the way to go. For example you changed:

            $html[] = '<a'
                . ' class="btn hasTooltip' . ($value ? '' : ' hidden') . '"'
                . ' href="index.php?option=com_categories&layout=modal&tmpl=component&task=category.edit&id=' . $value . '"'
                . ' target="_blank"'
                . ' title="' . JHtml::tooltipText('COM_CATEGORIES_EDIT_CATEGORY') . '" >'
                . '<span class="icon-edit"></span> ' . JText::_('JACTION_EDIT')
                . '</a>';

to:

$html[] = '<a class="btn hasTooltip' . ($value ? '' : ' hidden') . '" href="index.php?option=com_categories&layout=modal&tmpl=component&task=category.edit&id=' . $value . '" target="_blank" title="' . JHtml::tooltipText('COM_CATEGORIES_EDIT_CATEGORY') . '" ><span class="icon-edit"></span>' . JText::_('JACTION_EDIT') . '</a>';

That's definitely wrong (340 chars line). It's worse for readability and is against our coding standards (150 chars maximum). So please in your new PR try to avoid it.

Thanks!

@phproberto UGH.. it shouldn't be doing that... anyway I can pull the diff of the commits or do I need to manually go through every file?

after talking to George I'm closing this and starting over... its become a disaster. Sorry for all this.

Thanks roberto, i have no idea how it got like that, but I'll try to
double check it and if necessary "break" it myself.
Bear
On 10/23/2014 12:48, Roberto Segura wrote:

@N6REJ https://github.com/N6REJ I downloaded you branch and tried to
rebase it manually but I think you started from an outdated branch or
maybe the wrong branch (master?). I suggest you that you start from
scratch on a new branch (created from a new clean and updated staging)
just copying the files you really want to modify.

Also create huge lines is not the way to go. For example you changed:

         $html[]  =  '<a'
             .  ' class="btn hasTooltip'  .  ($value  ?  ''  :  ' hidden')  .  '"'
             .  ' href="index.php?option=com_categories&layout=modal&tmpl=component&task=category.edit&id='  .  $value  .  '"'
             .  ' target="_blank"'
             .  ' title="'  .  JHtml::tooltipText('COM_CATEGORIES_EDIT_CATEGORY')  .  '" >'
             .  '<span class="icon-edit"></span>'  .  JText::_('JACTION_EDIT')
             .  '</a>';

to:

$html[] = '' . JText::_('JACTION_EDIT') . '';

That's definitely wrong (340 chars line). It's worse for readability
and is against our coding standards (150 chars maximum). So please in
your new PR try to avoid it.

Thanks!

—
Reply to this email directly or view it on GitHub
#4372 (comment).

No virus found in this message.
Checked by AVG - www.avg.com http://www.avg.com
Version: 2015.0.5315 / Virus Database: 4181/8439 - Release Date: 10/23/14