User tests: Successful: Unsuccessful:
Pull Request for Issue # .
adds fontawesome's svg sprite sets to core.
provides a htmlhelper class to handle creating icons from sprites.
apply pr
run npm ci
check that joomla logo in admin hasn't changed looked.
inspect code notice it's now an inline svg, will fill support
svg images are not available in /media/vendor/fontawesome-free
svg sprites are used by calling the htmlhelper "sprite" class.
HTMLHelper::_('sprite', 'icon-joomla', 'brands', '', 'Joomla version', 'Icon showing Joomla\'s current version')
This class takes 5 arguments.
string $icon, string $brand = 'solid', string $class = '', string $title = '', string $description = ''
It returns a string consisting of the html to display the svg as mentioned here
default svg icon styling is set in 'administrator/templates/atum/scss/blocks/_icons.scss' by
svg.icon {
max-height: 2rem;
max-width: 2rem;
}
then individual icons are styled like this..
// Joomla
svg.icon-joomla {
fill: $white;
width: 1.5rem;
height: 1.5rem;
}
Status | New | ⇒ | Pending |
Category | ⇒ | Repository |
based on the proposal by dimitries we only need to ship the sprites files and extract the icon at runtime and include it in the body
@N6REJ please do not add the sprites. PHP is a capable language to concatenate some selected icons (that's what's a sprite)...
Edit: This PR shouldn't be merged unless there is an API endpoint for using the SVGs, something like #28351 . The files alone are useless without can official way to use them...
@N6REJ please do not add the sprites. PHP is a capable language to concatenate some selected icons (that's what's a sprite)...
Edit: This PR shouldn't be merged unless there is an API endpoint for using the SVGs, something like #28351 . The files alone are useless without can official way to use them...
extracting the icons we need from the sprites on the fly (and maybe cached) is better then ship 1000 additional files.
extracting the icons we need from the sprites on the fly (and maybe cached) is better then ship 1000 additional files.
Providing sprites means you're loosing (or to be more accurate making it way harder) the ability to simply override any icon by placing the responsive icon in the relative template folder (eg vendors/fontawsome/solid/star.svg). Pre caching can and should happen in the template...
if you do it right you can still override this icons, why should the extract process not look for the image first?
why should the extract process not look for the image first?
Why over engineer something that should be straight forward? The point is to work with the established workflows instead of creating a new one, especially for something so simple
You care about performance on client side, also care about performance on server and isp side....
Even if filesystems should handle many files good (because it's there job) they don't like many files, especially in one folder.
Also filetransfer protocols like brian mentioned don't like many files.
That's one of the reasons why some games created there own file "compression" format (actually without compression) because file access (read list close metadata) is a expensive.
You care about performance on client side, also care about performance on server and isp side
I'm not arguing with the server side caching, FWIW this was a public argument (twitter) over my proposal on the lazyloading (eg cache the attributes width and height instead of reading the images attributes in runtime, so it's kinda unfair to accuse me that I don't care of server side perf).
All I said was that the caching mechanism could (and probably should be) a concern of another part (eg Document or the template). I mean if you cache something you probably want (in the Joomla context) a GUI way to invalidate this cache at some point...
I think we talk about different things here, shipping additional 2000 files for so isn't a good option.
Anyway the PR on it's own is not useful.
To be clear about what I meant and to avoid confusion.
If you have to upload joomla to a server using ftp then adding an additional 2000 files has a massive impact on the time it takes as ftp transfers single files at a time. So there are 2000 open connections, 2000 transfers and 2000 close connections.
If you are unfortunate enough to be using windows locally then there is also a massive impact when you unzip as each file will be scanned on extraction.
Finally many hosts restrict their accounts not by disk size but by inodes. with lots of tiny files its easy to reach inode limits on some hosts.
I'm confused, you say DON"T include the sprites which are 3 files. Don't you mean don't include the svg's?
I have tested this item
I had a look at a few of the SVGs - read somewhere that they are to be preferred to fonts. Looks good. We will need some documentation on how to use them.
I think we talk about different things here, shipping additional 2000 files for so isn't a good option.
Anyway the PR on it's own is not useful.
It's not SUPPOSED to be useful on its on.. sheeesh.. its part of what @sandewt needs for his pr and what we will need in the icon team for the transition to svg
Labels |
Added:
?
|
you guys please come to a consensus or merge the pr.. one of the 2. I understand both sides of the arguments.
@N6REJ please do not add the sprites. PHP is a capable language to concatenate some selected icons (that's what's a sprite)...
Edit: This PR shouldn't be merged unless there is an API endpoint for using the SVGs, something like #28351 . The files alone are useless without can official way to use them...
as I said to harald this is the beginning of what will be done in the icon team.
as I said to harald this is the beginning of what will be done in the icon team.
Then you should explain that in the PR. As it is right now it serves no purpose at all. Not everyone is a mind reader or a member of your private club. Without an explanation of the need for this it is just fluff.
as I said to harald this is the beginning of what will be done in the icon team.
Then you should explain that in the PR. As it is right now it serves no purpose at all. Not everyone is a mind reader or a member of your private club. Without an explanation of the need for this it is just fluff.
not a private club.. you want to participate join JBS Icon Team in glip
Title |
|
moving this to draft while we sort out all the ramifications of which method to use.
Still no explanation of why this is needed
Having sprites (less file transfers / inode counts) and having scalable vector graphics as alternative (hopefully as replacement in future) is a good thing to have and not "fluff".
One of my customer uses a hoster which really have a problem providing a stable ftp connection on the shared host and everything that helps to copy Joomla as fast as possible into the webroot is really appreciated by me.
I was interested in how to change colour and came across this on stackeoverflow:
.icons i,
.icons svg {
color: #2759AE;
}
https://stackoverflow.com/questions/48608450/how-to-change-color-of-icons-in-font-awesome-5
After doing some research and talking to several people I'm removing sprites. I realize this will cause more files to be uploaded but I don't see a reasonable way around it.
Title |
|
Labels |
Added:
?
|
Labels |
Added:
?
Removed: ? |
Labels |
Added:
?
Removed: ? |
Labels |
Added:
?
Removed: ? |
@N6REJ can you revise the instructions now sprites have been removed if this is actually ready to be tested again (or is the debate still continuing?).
I pre-tested this yesterday afternoon by installing the download package, and only get the following folders in /media/vendor/fontawesome-free
I assume the single change in the commit is to then allow it to reference /media/vendor/fontawesome-free/images for the SVG files? If so, then the test is going to pass.
@particthistle yeah, it's simply to make the svg icons available. After this we can work on getting them used in the core.
part of the wait was waiting for #30707 to be merged. Then #31190 will be hopefully be the final piece to start making the transition
I have tested this item
/media/vendor/fontawesome-free/images now includes the SVG images.
Change to build/build-modules-js/settings.json looks to now allow the images to be accessed.
Excited to see FontAwesome available now in the core as it's a daily staple for any iconography I'm using.
@N6REJ - Adjust the summary of changes too at the top - you're not adding the svg sprite set now correct?
@particthistle thanks, missed a spot. Fixed now.
I have tested this item
All files now present and correct
I'm against adding 1600 files to the core which will not be used to 99,99%
agree with @HLeithner
@N6REJ would you mind explaining what the 1600 icons will be used for? This may allay concerns that others have here.
Will it replace something we already use and will it have advantages?
The long term goal, as has been asked for by several people, is to convert J! to using svg instead of icon fonts. This means making the complete FA font set available, as was brought up in #30793 the other day.
"I" don't know any way of easily handling dynamic loading of sprites. unless javascript is used, which I've been told is frowned upon.
From what I'm understanding the complaint is that we will have 1600 files to upload.
infact, FA states that using sprites is slower then files or fonts
From what I'm understanding the complaint is that we will have 1600 files to upload, not that there are 1600 files which there ALREADY ARE! It's built into /media/vendor/fontawesome. So I'm really not seeing the issue to be honest.
They're not
There are three issues with these 1600 files as already described above several times
@brianteeman I don't disagree but I'm not sure of a better way.
I'll see if I can figure a "system" out that will work.
Not everything is possible with a mass market cms
ok, I'll kick this off to draft mode while I see if there is a way I can make this work.
Title |
|
The long term goal, as has been asked for by several people, is to convert J! to using svg instead of icon fonts.
Some historie joomla/40-backend-template#441 (comment)
Labels |
Removed:
?
|
Category | Repository | ⇒ | Modules Administration Repository Libraries |
Title |
|
first attempt at creating a viable svg sprite icon
Category | Repository Modules Administration Libraries | ⇒ | Modules Administration Templates (admin) NPM Change Repository Libraries |
Labels |
Added:
NPM Resource Changed
|
Title |
|
Title |
|
I must be thick but where/what is the benefit of all this work
I must be thick but where/what is the benefit of all this work
the benefit comes from being able to use svg sprites for icons instead of icon fonts or icon images.
And the benefit of that is?
On Mon, 9 Nov 2020, 23:53 Bear, notifications@github.com wrote:
I must be thick but where/what is the benefit of all this work
the benefit comes from being able to use svg's for icons instead of icon
fonts or icon images.—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#31079 (comment),
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AAJ4P4IRFNNM7ZHI2WUUG53SPB6ITANCNFSM4SNQADQA
.
is its been asked for by many people
is its been asked for by many people
That doesn't answer my question
then ask other people please. I've given you my answer.
perhaps this will help you https://www.lambdatest.com/blog/its-2019-lets-end-the-debate-on-icon-fonts-vs-svg-icons/
then ask other people please. I've given you my answer.
some people asked for it is not an answer.
For reference - the accessibility argument is false. They can be accessible and contain useful information but generic svg such as these very rarely do.
For reference - the accessibility argument is false. They can be accessible and contain useful information but generic svg such as these very rarely do.
That is totally false. Icon Fonts fail miserably for people with dyslexia and there is an open issue for that: #30177
That issue is nothing to do with the use of an icon font per se
The accessibility benefit of using svg is not something that will just happen by switching to svg .
SVGs are armed with built in semantic elements – like < title > and < desc > that makes it accessible to screen reader and text browsers.
True but svg icons being added by this pr do not contain any semantic elements
Unlike icon fonts, SVGs are treated as an image and not as a text.
True but we already have the code in place to "hide" the text (although this PR proposes still using aria-hidden)
Moreover SVG is compatible with WAI-ARIA specifications – aria-labelledby attribute.
True but this PR doesn't implement that
Even more information about sprites.
https://fontawesome.com/how-to-use/on-the-web/advanced/svg-sprites
SVGs are armed with built in semantic elements – like < title > and < desc > that makes it accessible to screen reader and text browsers.
True but svg icons being added by this pr do not contain any semantic elements
I didn't add it cause I didn't want to create too many params. I've done so since it's apparently important
Moreover SVG is compatible with WAI-ARIA specifications – aria-labelledby attribute.
True but this PR doesn't implement that
aria-labelledby & aria-describedby would go in the span NOT the svg unless I'm mistaken. If they do belong in the svg then does aria-labelledby provide the tooltip that title does? Each of those override "<title>" & "<desc>"
either way its now supported.
Even more information about sprites.
https://fontawesome.com/how-to-use/on-the-web/advanced/svg-sprites
I had already included that in the docs
"It returns a string consisting of the html to display the svg as mentioned here"
True but svg icons being added by this pr do not contain any semantic elements
Because this PR is plain wrong. If the project wants to transition to SVG probably should base the work on my old PR: #28351 which was covering correctly these points (and much more)
it was made abundantly clear that including 1600+ icon files would not be supported so please don't rag on my pr just because you didn't want sprites and don't like me/my work.
Instead of you & brain complaining perhaps you could explain what would make it better and more importantly how/why. It's clear I'm not going to make both of you happy so I'm not even going to try unless the above criteria is applied.
@brianteeman ty for explaining what was wrong with the a11y part of it. I wondered about that but since you know far more about a11y then I do and hadn't mentioned it, I wasn't aware that was the issue.
it was made abundantly clear that including 1600+ icon files would not be supported so please don't rag on my pr just because you didn't want sprites and don't like me/my work
My pr was approved already by the project leader for j4, so although everybody is entitled to an opinion in many cases that opinion is irrelevant, in sort the number of files is irrelevant. Also the way you’re doing it here has very little advantages over the font, you still deliver whatever is given by font awesome (which is a lot of things that a user never asked for) so I’ll stick on what I said: the pr is wrong
Instead of you & brain complaining perhaps you could explain what would make it better and more importantly how/why
Fork my pr and then ask Ciaran and Allon for help
Also the way you’re doing it here has very little advantages over the font, you still deliver whatever is given by font awesome (which is a lot of things that a user never asked for) so I’ll stick on what I said: the pr is wrong
I don't disagree that it's simply extending fa to another icon type(?)
My understanding is J5 will be 100% FA so there's that. Changing my work to NON-FA only shouldn't be that hard ( one line ) when/if the time comes.
I don't disagree that it's simply extending fa to another icon type(?)
That’s not what I meant. Right now a page with only a user icon needs around 100kb of crap, although it only uses one icon... you didn’t fix that and that’s the performance part that needs fixing
I don't disagree that it's simply extending fa to another icon type(?)
That’s not what I meant. Right now a page with only a user icon needs around 100kb of crap, although it only uses one icon... you didn’t fix that and that’s the performance part that needs fixing
ok, & that is probably above my knowledge set. I do the best I can with the knowledge that I have. I'm almost always open to being taught a better way but I won't follow blindly. I need to understand the code I write if I can possibly do so.
I think you are getting a bit confused.
<span aria-hidden="true"><?php echo HTMLHelper::_('sprite', 'icon-joomla', 'brands', '', 'Joomla version', 'Icon showing Joomla\'s current version'); ?></span>
Joomla Version is a title
Icon showing Joomla's current version is a description
The purpose of the title and the description is to provide information about the svg image
Think of the <title>
as the alt text of the svg and <desc>
as the longer description of the svg
The desc
is never visually displayed and the title
may be visually displayed as a tooltip (depending on the browser settings)
They are both however exposed to the accessibility api of the browser which is great and means that the text can be announced by a screen reader.
However in this code you have wrapped the svg inside a span with aria-hidden=true
. That code makes everything inside the span hidden from the accessibility api. Which completely negates all your work adding the title
and desc
[the reason the icon fonts are wrapped in that span is because they are fonts and therefore would be recognised as text by a screen reader which would announce garbage]
Final comment is that of course you need to make sure the text is translatable so you need to use language constants and pass them through JText
Oops forgot to add that we also have to consider if there is any benefit to the icon being described or is it just decorative.
If it is just decorative then you probably dont want to have it announced.
Think of it like the current markup.
an icon and visually displayed text.
In this example you would probably not want the svg title as the svg is purely decorative
an icon and visually hidden text (class=sr-only
).
In this example you would probably want the svg title and can remove the hidden text
[the reason the icon fonts are wrapped in that span is because they are fonts and therefore would be recognised as text by a screen reader which would announce garbage]
considering svgs are treated as images by the browser thats probably not what we want then.
do you have a suggestion on how to make it work well with a11y?
There is no one size fits all solution
Based on the current code base I would say that everywhere we have text in an sr-only span that can be deleted and used as the title element of the svg and you remove the aria-hidden on the svg
but everywhere that the text is not hidden then you keep the aria-hidden and dont add a title.
quick examples below - its late so I havent tested the code but its just there as an illustration
<div class="mr-2">
<span class="icon-wifi" aria-hidden="true"></span>
Blog Sample Data
</div>
So here you would put
<div class="mr-2">
<span aria-hidden="true"><?php echo HTMLHelper::_('sprite', 'icon-wifi', 'solid'); ?></span>
Blog Sample Data
</div>
<button type="button" data-toggle="dropdown" aria-haspopup="true" aria-expanded="false" class="btn" id="dropdownMenuButton-87">
<span class="icon-cog" aria-hidden="true"></span>
<span class="sr-only">Edit the 'Sample Data' module</span>
</button>
So here you would put
<button type="button" data-toggle="dropdown" aria-haspopup="true" aria-expanded="false" class="btn" id="dropdownMenuButton-87">
<?php echo HTMLHelper::_('sprite', 'icon-wifi', 'solid', '','Edit the 'Sample Data' module'); ?>
</button>
As stated privately - the small performance benefits do not justify the amount of work involved in making this change throughout the codebase
As I know your well aware its a crap ton of work... already a yr in the making...
considering what your saying perhaps the best thing to do, at least for now, is have the ability in place ( that work is basically already done ) but NOT start changing things till PLT says do it.
So, Had a productive conversation in JBS @ 0300 regarding this pr and it was pretty much decided that without being able to have individual svg icons like webfonts or images that it's not practical to use svg in this way.
As brian stated, converting to all svg would be a minimum 3 mo venture due to the enormous amount of work that would be involved.
IF one icon from each icon set ( solid, brands, regular ) were loaded it would result in 2.8mb of files being downloaded for as little as 3 icons.
Thats not practical. So for that reason I'm closing this pr and advising icon team that we're done going down this rabbit hole. If someone creates a PLT accepted way of handing svg icon SETS we'll go back at it.
Status | Pending | ⇒ | Closed |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2020-11-11 19:06:32 |
Closed_By | ⇒ | N6REJ |
If someone creates a PLT accepted way of handing svg icon SETS we'll go back at it.
There was an accepted way as demoed in #28351 The reason that I closed that PR was because there wasn't any common ground on the implementation of the helper (jLayout, or HTMLHelper, or JDocument function). I've told you already this on our private talk like months ago but you didn't (and it seems you still don't want to) listen...
do we really want to add an additional 2000 files to the core. ftp users will love that