[ticket/15538] Twig icon function#5545
Conversation
|
Couple of things to discuss: Currently using the Therefore suppression is needed, currently simply done by Moreover, eventhough I am currently using the
And lastly, currently there are two instances of where I catch |
|
!set WIP 🚧 |
|
Then I still need to know what elements and attributes to remove / replace for SVG's? Also, in an other comment that you would also like to use this for forum icons, I'm guessing you mean "New Post", "Forum locked", etc with that. Why would they go in a different directory than the theme? It sounds like they are very closely tight to the actual theme and currently a lot of them already use different ones. Instead, perhaps you can just add a class to the icon like |
|
Any and all fill rules are ok. You not not need to sanitize any of the content other than the svg tag and the title needs to be dynamic as you have done. The rest of the contents just pass through. This will be used for everything potentially which includes: |
|
So talking it over as I mentioned on your other PR this would need to be |
|
Don't add fill rule to css!
As for attributes removal: title attribute must be removed because it is used as tooltip in some browsers. It overrides title from parent element, so for example if you have this: tooltip will show "SVG title!" instead of "Link title!" as tooltip. Best option for cleaning up is to run SVGO with all options to clear everything. |
|
|
SVGO can be ran before uploading icons. There is also web interface for it: https://jakearchibald.github.io/svgomg/ As for bandwidth, I mean including icons on every page load. If icons are used for all decorations instead of fonts, that could be a lot of icons per page. Its one of reasons why glyph fonts became so popular - fonts use minimal markup, reducing page loading times compared to embedding SVG. |
|
🤔 calling 💩 on that fonts were introduced because of a lack of support of svg. Massive html pages load way faster than dynamic pages as it’s all cached and gzipped. There is a negative return actually. Let’s say the entire page was 2mb which is massive for an html page. That’s only on initial load subsequent loads would be like 10kb tops with gzip and cache. And there is no way it’s anywhere near that size unless you are trying to load every emoji on a page. |
|
Please read what I posted again. I said "one of reasons". Lack of support for SVG was definitely main reason though. Lets do some math. FontAwesome 5 SVG size is between 300 for most basic icon and 1k for most complex icon. So 650b on average. Those are very simple icons. EmojiOne icons are mostly several kb in size up to 11kb. Those are shiny icons used for decorations, such as forum icons. 11kb is extreme icon, on average its about 2-3kb, so 2.5kb. phpBB.com forum index is currently 66.5kb. Lets calculate replacing icons with SVG embedded in document:
Big forum icons should be shiny, so that's similar to EmojiOne icons. Other icons are basic, so similar to FontAwesome 5 icons. That's on average 650b for basic icon, 2.5kb for forum icon. Let's assume minimum permissions, so in navigation only 3 icons. Header: 4 * 0.65 = 2.6kb That's extra 86.5kb for page that's currently 66.5kb. Download size is more than doubled. Its added to every page load. In forum view there are even more icons. In posts there are lots of buttons in every post. Today page load is very important. It affects SEO. For me that's a massive no for usage. |
|
🤔 Well prosilver is an irrelavent exp, will have to build some test I guess inthe new theme to figure everythingout. |
|
Cool. Please post results, it would be interesting to know how those calculations work for similar size forum with new layout. 🙂 |
|
done. Tests run through Chrome Audit 'Sim Fast 3G, 4x CPU Throttle - Mobile' Note:
I did convert over to use fully inline svgs, using the themes current icon pack
Results: New Theme - svg unoptimized (express static gzip) New Theme - iconify for non-custom icons (express static gzip) phpbb.com (local static tests loaded assets from live sites) area51 (local static tests loaded assets from live sites) |
|
ok so finished and updated the above post with the results. sort of as I expected there is not really much tangible difference with static files. would really need to test with phpbb page renders. But the numbers are essentially vacuumed meaning that from a performance gain either way is under 2 sec loads with 10ms interactivity times. Compare this to current static versions of .com and area51 at 3+ secs vs live versions and you see a consistent extra sec for area51 and 2-3 secs for .com I would not mind entertaining a 3.3 prosilver convert to iconify over font-awesome given these results. I also put up some tests for that as well prosilver integration: https://codepen.io/hanakin/pen/oVyvBp?editors=1100 @mrgoldy to do all this I ending up building a nunjucks filter for all the icon loading in current chameleon repo environment.addFilter('icon', (icon, type, classlist, hidden = false)and called it like so {{ 'sort-ascending'|icon('inline', 'o-icon c-toolbar-action-icon t-toolbar-action-icon')|safe }}seems to work fairly well but its all hardcoded |
|
That is not real forum test, it is missing what forums actually have - content. You didn't replace forum icon, which is biggest icon and you've made only 2 rows. Those are conditions of a dead forum. Real forums use 20 rows, not 2. That's 10 times more replication of big icons. Try replacing forum icon with svg, which will happen in new theme and add more rows to see conditions of actual forum. |
|
That was not the test that is a planning environment I put up. The tests can be seen in branch 0.2.0 of chameleon on my account https://github.com/hanakin/chameleon/tree/0.2.0 |
|
Oh, ok. Now it makes sense |
|
@mrgoldy check out this Seems macros are still able to be used |
|
I am not sure if this approach will work.. Will investigate some more. |
| $web_path = $board_url ? generate_board_url() . '/' : $environment->get_web_root_path(); | ||
| $style_path = $this->user->style['style_path']; | ||
|
|
||
| return "{$web_path}styles/{$style_path}/template/icons/png/{$icon}.png"; |
There was a problem hiding this comment.
Okay, btw this will not handle 'style inheritance'. Is that correct behaviour?
There was a problem hiding this comment.
If the icon is not present in the user's style it will display the 'No image'.
It will not inherit from any parent style (eg. Chameleon).
|
I tried to rebase, resolve conflicts and force push. |
PHPBB3-15538
|
pending the test looks good |
|
ill work on the icon rework for prosilver pr based off this tomorrow probably so we can merge both together. |
|
@mrgoldy just noticed that you have all the templates in the template folder. This is not right the icons folder needs moved to all/templates/macros |
hanakin
left a comment
There was a problem hiding this comment.
noticed some minor changes that need to be made to facilitate using different icons.
PHPBB3-15538
PHPBB3-15538
PHPBB3-15538
| @@ -0,0 +1,4 @@ | |||
| {% spaceless %} | |||
There was a problem hiding this comment.
think these are supposed to be in template not templates
There was a problem hiding this comment.
they should also be twig files not html files
There was a problem hiding this comment.
I mean .. #5545 (comment)
@mrgoldy just noticed that you have all the templates in the template folder. This is not right the icons folder needs moved to all/templates/macros
If you let me know where you want them I’ll adjust it. Along with the twig file extension.
There was a problem hiding this comment.
it belongs in the current folder that exsists in the software https://github.com/phpbb/phpbb/tree/master/phpBB/styles/all/template
There was a problem hiding this comment.
templates was a typo so just move the macros/iocns folders into the template folder
| * {'data-ajax': 'mark_forums'} results in ' data-ajax="mark_forums"' | ||
| * @return string | ||
| */ | ||
| public function icon(environment $environment, $type, $icon, $title = '', $hidden = false, $classes = '', array $attributes = []) |
There was a problem hiding this comment.
So, currently you need to explicitly define icon type to render it. Can we add some auto mode to automatically detect icon type and render it then? Or even split such code into a function and then use it here instead of switch.
There was a problem hiding this comment.
No that makes no sense and defeats the purpose. How would you even automatically detect an icon type? There are too different. I suppose you could make one a default? If it finds an svg use it unless defined
There was a problem hiding this comment.
Automatically detecting will also not return one definitive result, as a lot of the icon sets use the same names. So, that would only be possible indeed if some 'default' was configured. And then you can just use that variable in the template instead: {{ icon(DEFAULT_ICON, 'user') }}, which perhaps then can moved to the back-end indeed. Just a thought.
PHPBB3-15538
Checklist:
Tracker ticket (set the ticket ID to your ticket ID):
https://tracker.phpbb.com/browse/PHPBB3-15538
For usage in the Chameleon style.
Reference PR's: #5489 & #4765
The only thing to be added from this PR is the twig extension (declaration and class), the rest is for testing purposes and the icon templates should go in the Chameleon style.