NPM Resource Changed ? Success

User tests: Successful: Unsuccessful:

avatar N6REJ
N6REJ
12 Oct 2020

Pull Request for Issue # .

Summary of Changes

adds fontawesome's svg sprite sets to core.
provides a htmlhelper class to handle creating icons from sprites.

Testing Instructions

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

Actual result BEFORE applying this Pull Request

svg images are not available in /media/vendor/fontawesome-free
image
image

Expected result AFTER applying this Pull Request

image
image

Documentation Changes Required

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 = ''

  • icon name. ( can be any name but if icon- is included then it is stripped )
  • sprite set name ( brands, regular or solid ) ( 'solid' by default )
  • class ( 'icon' + icon name by default )(optional)
  • title ( used for tooltip and a11y ) ( optional )
  • description ( used by a11y ) (optional)

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;
}
avatar N6REJ N6REJ - open - 12 Oct 2020
avatar N6REJ N6REJ - change - 12 Oct 2020
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 12 Oct 2020
Category Repository
avatar brianteeman
brianteeman - comment - 12 Oct 2020

do we really want to add an additional 2000 files to the core. ftp users will love that

avatar HLeithner
HLeithner - comment - 13 Oct 2020

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

avatar sandewt
sandewt - comment - 13 Oct 2020

I tested successfully, but how to change te color of the star.svg ? Should a new file, for example star_color.svg, be created?

issue_31079

.content_rating .star {
    display: inline-block;
    width: 1em;
    height: 1em;
    content: "";
    background-image: url(star.svg);
    background-size: 110%;
avatar dgrammatiko
dgrammatiko - comment - 13 Oct 2020

@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...

avatar HLeithner
HLeithner - comment - 13 Oct 2020

@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.

avatar dgrammatiko
dgrammatiko - comment - 13 Oct 2020

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...

avatar HLeithner
HLeithner - comment - 13 Oct 2020

if you do it right you can still override this icons, why should the extract process not look for the image first?

avatar dgrammatiko
dgrammatiko - comment - 13 Oct 2020

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 ?

avatar HLeithner
HLeithner - comment - 13 Oct 2020

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.

avatar dgrammatiko
dgrammatiko - comment - 13 Oct 2020

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...

avatar HLeithner
HLeithner - comment - 13 Oct 2020

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.

avatar brianteeman
brianteeman - comment - 13 Oct 2020

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.

avatar N6REJ
N6REJ - comment - 13 Oct 2020

I'm confused, you say DON"T include the sprites which are 3 files. Don't you mean don't include the svg's?

avatar N6REJ
N6REJ - comment - 13 Oct 2020

@sandewt hold off till we get this merged then we'll work on your pr if thats ok.

avatar ceford ceford - test_item - 14 Oct 2020 - Tested successfully
avatar ceford
ceford - comment - 14 Oct 2020

I have tested this item successfully on 8eb52e8

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.


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

avatar sandewt
sandewt - comment - 14 Oct 2020

@sandewt hold off till we get this merged then we'll work on your pr if thats ok.

I'm waiting. Although a PR for this PR can quickly be realized (with or without the improvement proposal).

avatar N6REJ
N6REJ - comment - 14 Oct 2020

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

avatar N6REJ N6REJ - change - 14 Oct 2020
Labels Added: ?
avatar N6REJ
N6REJ - comment - 14 Oct 2020

you guys please come to a consensus or merge the pr.. one of the 2. I understand both sides of the arguments.

avatar N6REJ
N6REJ - comment - 14 Oct 2020

@sandewt hold off till we get this merged then we'll work on your pr if thats ok.

I'm waiting. Although a PR for this PR can quickly be realized (with or without the improvement proposal).

whichever you want to do, I'm trying to be helpful

avatar N6REJ
N6REJ - comment - 14 Oct 2020

@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.

avatar brianteeman
brianteeman - comment - 14 Oct 2020

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.

avatar N6REJ N6REJ - change - 15 Oct 2020
The description was changed
avatar N6REJ N6REJ - edited - 15 Oct 2020
avatar N6REJ
N6REJ - comment - 15 Oct 2020

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

avatar N6REJ N6REJ - change - 15 Oct 2020
Title
[4.0] Add FA svg set(s)
[4.0] [DRAFT] Add FA svg set(s)
avatar N6REJ N6REJ - edited - 15 Oct 2020
avatar N6REJ
N6REJ - comment - 15 Oct 2020

moving this to draft while we sort out all the ramifications of which method to use.

avatar brianteeman
brianteeman - comment - 15 Oct 2020

Still no explanation of why this is needed

avatar jiweigert
jiweigert - comment - 15 Oct 2020

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.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/31079.
avatar ceford
ceford - comment - 16 Oct 2020

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


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

avatar N6REJ
N6REJ - comment - 18 Oct 2020

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.

avatar Quy Quy - change - 22 Oct 2020
Title
[4.0] [DRAFT] Add FA svg set(s)
[4.0] Add FA svg set(s)
avatar Quy Quy - edited - 22 Oct 2020
avatar N6REJ N6REJ - change - 24 Oct 2020
Labels Added: ?
avatar N6REJ N6REJ - change - 29 Oct 2020
Labels Added: ?
Removed: ?
avatar N6REJ N6REJ - change - 31 Oct 2020
Labels Added: ?
Removed: ?
avatar N6REJ N6REJ - change - 1 Nov 2020
Labels Added: ?
Removed: ?
avatar particthistle
particthistle - comment - 2 Nov 2020

@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

image

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.

avatar N6REJ N6REJ - change - 2 Nov 2020
The description was changed
avatar N6REJ N6REJ - edited - 2 Nov 2020
avatar N6REJ
N6REJ - comment - 2 Nov 2020

@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

avatar particthistle particthistle - test_item - 3 Nov 2020 - Tested successfully
avatar particthistle
particthistle - comment - 3 Nov 2020

I have tested this item successfully on 5a988d3

/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?


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

avatar N6REJ N6REJ - change - 3 Nov 2020
The description was changed
avatar N6REJ N6REJ - edited - 3 Nov 2020
avatar N6REJ
N6REJ - comment - 3 Nov 2020

@particthistle thanks, missed a spot. Fixed now.


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

avatar softforge softforge - test_item - 7 Nov 2020 - Tested successfully
avatar softforge
softforge - comment - 7 Nov 2020

I have tested this item successfully on 5a988d3

All files now present and correct


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

avatar HLeithner
HLeithner - comment - 7 Nov 2020

I'm against adding 1600 files to the core which will not be used to 99,99%

avatar brianteeman
brianteeman - comment - 7 Nov 2020

agree with @HLeithner

avatar softforge
softforge - comment - 7 Nov 2020

@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?


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

avatar N6REJ
N6REJ - comment - 7 Nov 2020

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

avatar brianteeman
brianteeman - comment - 7 Nov 2020

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

  1. Makes the unzip of joomla 2-3x slower on windows
  2. Makes the upload/download of a joomla site considerably slower with ftp file transfer
  3. Uses up a lot of inodes on a shared hosting account which may be limited
avatar N6REJ
N6REJ - comment - 7 Nov 2020

@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.

avatar brianteeman
brianteeman - comment - 8 Nov 2020

Not everything is possible with a mass market cms

avatar N6REJ
N6REJ - comment - 8 Nov 2020

ok, I'll kick this off to draft mode while I see if there is a way I can make this work.

avatar N6REJ N6REJ - change - 8 Nov 2020
Title
[4.0] Add FA svg set(s)
[4.0] [DRAFT] Add FA svg set(s)
avatar N6REJ N6REJ - edited - 8 Nov 2020
avatar sandewt
sandewt - comment - 8 Nov 2020

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)

avatar N6REJ N6REJ - change - 8 Nov 2020
Labels Removed: ?
avatar joomla-cms-bot joomla-cms-bot - change - 8 Nov 2020
Category Repository Modules Administration Repository Libraries
avatar N6REJ N6REJ - change - 8 Nov 2020
Title
[4.0] [DRAFT] Add FA svg set(s)
[4.0] [DRAFT][WIP] Add FA svg set(s)
avatar N6REJ N6REJ - edited - 8 Nov 2020
avatar N6REJ
N6REJ - comment - 8 Nov 2020

first attempt at creating a viable svg sprite icon

avatar joomla-cms-bot joomla-cms-bot - change - 9 Nov 2020
Category Repository Modules Administration Libraries Modules Administration Templates (admin) NPM Change Repository Libraries
avatar N6REJ N6REJ - change - 9 Nov 2020
Labels Added: NPM Resource Changed
avatar N6REJ N6REJ - change - 9 Nov 2020
Title
[4.0] [DRAFT][WIP] Add FA svg set(s)
[4.0] Add FA svg set(s)
avatar N6REJ N6REJ - edited - 9 Nov 2020
avatar N6REJ N6REJ - change - 9 Nov 2020
Title
[4.0] Add FA svg set(s)
[4.0] Add FA svg sprite set(s) & helperclass
avatar N6REJ N6REJ - edited - 9 Nov 2020
avatar N6REJ N6REJ - change - 9 Nov 2020
The description was changed
avatar N6REJ N6REJ - edited - 9 Nov 2020
avatar brianteeman
brianteeman - comment - 9 Nov 2020

I must be thick but where/what is the benefit of all this work

avatar N6REJ
N6REJ - comment - 9 Nov 2020

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.

avatar N6REJ N6REJ - change - 10 Nov 2020
The description was changed
avatar N6REJ N6REJ - edited - 10 Nov 2020
avatar brianteeman
brianteeman - comment - 10 Nov 2020

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
.

avatar N6REJ
N6REJ - comment - 10 Nov 2020

is its been asked for by many people

avatar brianteeman
brianteeman - comment - 10 Nov 2020

is its been asked for by many people

That doesn't answer my question

avatar N6REJ
N6REJ - comment - 10 Nov 2020

then ask other people please. I've given you my answer.

avatar brianteeman
brianteeman - comment - 10 Nov 2020

then ask other people please. I've given you my answer.

some people asked for it is not an answer.

avatar brianteeman
brianteeman - comment - 10 Nov 2020

For reference - the accessibility argument is false. They can be accessible and contain useful information but generic svg such as these very rarely do.

avatar dgrammatiko
dgrammatiko - comment - 10 Nov 2020

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

avatar N6REJ N6REJ - change - 10 Nov 2020
The description was changed
avatar N6REJ N6REJ - edited - 10 Nov 2020
avatar brianteeman
brianteeman - comment - 10 Nov 2020

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

avatar dgrammatiko
dgrammatiko - comment - 10 Nov 2020

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)

avatar sandewt
sandewt - comment - 10 Nov 2020
avatar N6REJ
N6REJ - comment - 10 Nov 2020

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.

avatar N6REJ N6REJ - change - 10 Nov 2020
The description was changed
avatar N6REJ N6REJ - edited - 10 Nov 2020
avatar N6REJ
N6REJ - comment - 10 Nov 2020

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"

avatar N6REJ
N6REJ - comment - 10 Nov 2020

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.

avatar dgrammatiko
dgrammatiko - comment - 10 Nov 2020

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

avatar dgrammatiko
dgrammatiko - comment - 10 Nov 2020

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

avatar N6REJ
N6REJ - comment - 10 Nov 2020

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.

avatar dgrammatiko
dgrammatiko - comment - 10 Nov 2020

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

avatar N6REJ
N6REJ - comment - 10 Nov 2020

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.

avatar brianteeman
brianteeman - comment - 10 Nov 2020

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

avatar brianteeman
brianteeman - comment - 10 Nov 2020

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.

  1. an icon and visually displayed text.
    In this example you would probably not want the svg title as the svg is purely decorative

  2. 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

avatar N6REJ
N6REJ - comment - 10 Nov 2020

@brianteeman

[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?

avatar brianteeman
brianteeman - comment - 11 Nov 2020

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

Example 1

<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>

Example 2

<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>
avatar brianteeman
brianteeman - comment - 11 Nov 2020

As stated privately - the small performance benefits do not justify the amount of work involved in making this change throughout the codebase

avatar N6REJ
N6REJ - comment - 11 Nov 2020

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.

avatar N6REJ
N6REJ - comment - 11 Nov 2020

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.

avatar N6REJ N6REJ - change - 11 Nov 2020
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2020-11-11 19:06:32
Closed_By N6REJ
avatar N6REJ N6REJ - close - 11 Nov 2020
avatar dgrammatiko
dgrammatiko - comment - 11 Nov 2020

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...

Add a Comment

Login with GitHub to post a comment