[ticket/16237] Overhaul icons to use Twig function by hanakin · Pull Request #5753 · phpbb/phpbb

hanakin · 2019-11-30T10:07:13Z

Checklist:

Correct branch: master for new features; 3.3.x & 3.2.x for fixes
Tests pass
Code follows coding guidelines: master, 3.3.x and 3.2.x
Commit follows commit message format

PHPBB3-16237

Depends on: #5545

https://tracker.phpbb.com/browse/PHPBB3-16237

TODO:

Rework ajax icon swaps for subscriptions
Fix blockquote icon
Rework new svg logo inclusion
Fix return arrow icons

Screenshot examples:

.gitignore

phpBB/includes/functions.php

phpBB/includes/mcp/mcp_forum.php

phpBB/styles/prosilver/template/forumlist_body.html

phpBB/styles/prosilver/template/mcp_forum.html

ghost

Was able to do 42/94 files for now, will continue when I have some more time.
Reference to self: viewtopic_body.html

phpBB/styles/prosilver/template/forumlist_body.html

phpBB/styles/prosilver/template/memberlist_view.html

phpBB/styles/prosilver/template/navbar_footer.html

ghost · 2020-05-03T06:36:56Z

phpBB/styles/prosilver/template/navbar_header.html

 			<li data-last-responsive="true">
 				<a href="{U_ACP}" title="{L_ACP}" role="menuitem">
-					<i class="icon fa-cogs fa-fw" aria-hidden="true"></i><span>{L_ACP_SHORT}</span>
+					{{ Icon('iconify', 'fa:cogs', '', false) }}<span>{{ lang('ACP_SHORT') }}</span>


Not sure if you tried this before or after you removed the lang() from macros/icon/iconify.twig, but it should work correctly after that. The problem was that ACP_SHORT returns ACP, which was than translated again by the macro, into the long version.

ghost · 2020-05-03T06:40:30Z

phpBB/styles/prosilver/template/navbar_header.html

 						<li>
 							<a href="{U_LOGIN_LOGOUT}" title="{L_LOGIN_LOGOUT}" accesskey="x" role="menuitem">
-								<i class="icon fa-power-off fa-fw" aria-hidden="true"></i><span>{L_LOGIN_LOGOUT}</span>
+								{{ Icon('iconify', 'fa:power-off', lang('LOGIN_LOGOUT'), false) }}


This will need the L_ prefix again, as it's created PHP side. Alternatively you can just check if the current user is a guest and display the correct language string depending on that.

Same here and below.

yes the L is not possible

ghost · 2020-05-03T06:49:08Z

phpBB/styles/prosilver/template/search_results.html

+								'mdi:file-document-outline' : true,
+							}, '', true, 'c-forum-row-icon') }}
+						</a>
+						{% if lang(searchresults.TOPIC_AUTHOR) == S_USERNAME %}


Any chance we could move this to a PHP check instead?
So we can perform a proper one instead: S_THIS_USER => $user->data['user_id'] == $row['topic_poster'], or some other variable name, rather than comparing usernames. Or alternatively assign the poster_id/topic_poster to the template and compare that: {% if searchresults.POSTER_ID == user.data.id %}

i open to whatever is best. but it php is not my game!

just let me know where best to put it and can do

Perhaps it's easier for now to tag it with a @todo mrgoldy and I'll add a PR afterwards, cause the check is in various places / files.

phpBB/styles/all/template/macros/icons/font.twig

phpBB/styles/prosilver/template/ucp_pm_message_header.html

phpBB/styles/prosilver/template/ucp_pm_viewmessage.html

phpBB/styles/prosilver/template/viewforum_body.html

phpBB/styles/prosilver/template/forumlist_body.html

phpBB/styles/prosilver/template/ucp_pm_viewmessage.html

phpBB/styles/prosilver/template/viewtopic_body.html

phpBB/styles/prosilver/template/viewtopic_topic_tools.html

ghost · 2020-05-04T07:34:54Z

phpBB/styles/prosilver/template/viewtopic_topic_tools.html

 				<li>
 					<a href="{U_EMAIL_TOPIC}" title="{L_EMAIL_TOPIC}">
-						<i class="icon fa-envelope-o fa-fw" aria-hidden="true"></i><span>{L_EMAIL_TOPIC}</span>
+						{{ Icon('iconify', 'mdi:send-circle-outline', '', true) }}<span>{L_EMAIL_TOPIC}</span>


Looks like this can be added to the title parameter.

phpBB/styles/prosilver/template/viewtopic_topic_tools.html

phpBB/styles/prosilver/template/search_results.html

ghost · 2020-05-05T14:54:55Z

phpBB/styles/prosilver/template/viewtopic_body.html

-							</a>
-						</li>
-					<!-- ENDIF -->
+					<!-- INCLUDE posting_buttons.html -->


What you include here, are the posting buttons, which are the BBCodes shown above the textarea when creating a post.
You include them again, in the dropdown version.

hmm looks like all the changes I made to factor out all the JS responsive menu bulllshit got stripped out so ill have to fix all of this also.

phpBB/phpbb/template/twig/extension/icon.php

ghost · 2020-05-07T11:20:33Z

phpBB/styles/prosilver/template/navbar_footer.html

 			<span class="crumb">
 				<a href="{U_INDEX}" data-navbar-reference="index">
-					<!-- IF not U_SITE_HOME --><i class="icon fa-home fa-fw" aria-hidden="true"></i><!-- ENDIF --><span>{L_INDEX}</span>
+					<!-- IF not U_SITE_HOME -->{{ Icon('iconify', 'fa:home', lang(L_INDEX), false) }}<!-- ENDIF -->


Now you remove the text aswell when there is a 'Main website'. The check is only if an icon should be displayed.

Suggested change

{{ Icon('iconify', 'fa:home', lang(L_INDEX), false) }}

{% if U_SITE_HOME %}{{ lang('INDEX') }}{% else %}{{ Icon('iconify', 'fa:home', lang('INDEX'), false) }}{% endif %}

fixed

ghost · 2020-05-08T13:23:14Z

phpBB/styles/prosilver/template/ucp_pm_message_header.html

 						<li>
 							<a href="{U_PRINT_PM}" title="{L_PRINT_PM}" accesskey="p">
-								<i class="icon fa-print fa-fw" aria-hidden="true"></i><span>{L_PRINT_PM}</span>
+								{{ Icon('iconify', 'fa:print', lang('PRINT_PM'), false) }}<span>{L_}</span>


Left over L_.

ghost · 2020-05-08T13:24:48Z

phpBB/styles/prosilver/template/ucp_pm_history.html


 			<p class="author">
-				<span><i class="icon fa-file fa-fw icon-lightgray icon-md" aria-hidden="true"></i><span class="sr-only">{history_row.MINI_POST}</span></span> {L_SENT_AT}{L_COLON} <strong>{history_row.SENT_DATE}</strong>
+				<span title="{history_row.MINI_POST}">{{ Icon('iconify', 'fa:file', history_row.MINI_POST, true, 'c-topic-icon') }}</span> {L_SENT_AT}{L_COLON} <strong>{history_row.SENT_DATE}</strong>


The title attribute here is rather redundant on the icon here, as it's exactly the same as the parent  that only has one child.

ghost · 2020-05-08T13:32:36Z

phpBB/styles/prosilver/template/simple_footer.html

 <script src="{T_ASSETS_PATH}/javascript/core.js?assets_version={T_ASSETS_VERSION}"></script>
 <!-- INCLUDEJS forum_fn.js -->
 <!-- INCLUDEJS ajax.js -->
+<script src="https://code.iconify.design/1/1.0.0-rc7/iconify.min.js"></script>


btw, is this still the correct version? Can we not use 1.0.6?
https://code.iconify.design/1/1.0.6/iconify.min.js

Needs to be updated but also we need to serve one version of this from assets and only serve from CDN if this is enabled in the ACP.

We can also create a new ticket + PR for that part though to not needlessly increase the size of this PR.

ghost · 2020-05-08T13:33:33Z

phpBB/styles/prosilver/template/search_results.html

 			<li>
 				<a href="{searchresults.U_VIEW_POST}" class="arrow-{S_CONTENT_FLOW_END}">
-					<i class="icon fa-angle-{S_CONTENT_FLOW_END} fa-fw icon-black" aria-hidden="true"></i><span>{L_JUMP_TO_POST}</span>
+					{{ Icon('iconify', 'fa:angle'-S_CONTENT_FLOW_END, lang('JUMP_TO_POST'), false, 'c-return-arrow-icon') }}


This is wrongly concatenated

Suggested change

{{ Icon('iconify', 'fa:angle'-S_CONTENT_FLOW_END, lang('JUMP_TO_POST'), false, 'c-return-arrow-icon') }}

{{ Icon('iconify', 'fa:angle-' ~ S_CONTENT_FLOW_END, lang('JUMP_TO_POST'), false, 'c-return-arrow-icon') }}

ghost · 2020-05-08T13:34:47Z

phpBB/styles/prosilver/template/search_results.html

+								<!-- IF searchresults.ATTACH_ICON_IMG -->{{ Icon('iconify', 'fa:paperclip', '', true) }}<!-- ENDIF -->
 								{% EVENT topiclist_row_topic_by_author_before %}
-								{L_POST_BY_AUTHOR} <!-- EVENT search_results_topic_author_username_prepend -->{searchresults.TOPIC_AUTHOR_FULL}<!-- EVENT search_results_topic_author_username_append --> &raquo; <time datetime="{searchresults.FIRST_POST_TIME_RFC3339}">{searchresults.FIRST_POST_TIME}</time> &raquo; {L_IN} <a href="{searchresults.U_VIEW_FORUM}">{searchresults.FORUM_TITLE}</a>
+								{L_POST_BY_AUTHOR} <!-- EVENT search_results_topic_author_username_prepend -->{searchresults.TOPIC_AUTHOR_FULL}<!-- EVENT search_results_topic_author_username_append --> &raquo; {searchresults.FIRST_POST_TIME} &raquo; {L_IN} <a href="{searchresults.U_VIEW_FORUM}">{searchresults.FORUM_TITLE}</a>


Why is the <time> element removed here?

merge error?

ghost · 2020-05-08T13:39:56Z

phpBB/styles/prosilver/template/posting_buttons.html

-		<i class="icon fa-link fa-fw" aria-hidden="true"></i>
-	</button>
-	<!-- ENDIF -->
-	<!-- IF S_BBCODE_FLASH -->


Why is the Flash BBCode removed?

I could have sworn we dropped support for it

We dropped it for Flash attachments, if I recall correctly. Not the BBCode.

ghost · 2020-05-08T13:50:33Z

phpBB/styles/prosilver/template/mcp_post.html

 		</p>
 		<!-- ELSE -->
-		<p class="author"><span><i class="icon fa-file fa-fw icon-lightgray icon-md" aria-hidden="true"></i><span class="sr-only">{MINI_POST_IMG}</span></span> {L_POSTED} {L_POST_BY_AUTHOR} {POST_AUTHOR_FULL} &raquo; {POST_DATE}</p>
+		<p class="author"><span>{{ Icon('iconify', 'fa:file', MINI_POST_IMG, true,  'c-topic-icon') }}</span> {L_POSTED} {L_POST_BY_AUTHOR} {POST_AUTHOR_FULL} &raquo; {POST_DATE}</p>


The MINI_POST_IMG can not be the title, as that is not actually a language string but a  HTML element with specific classes.

seems this needs a new template variable

ghost · 2020-05-08T13:51:20Z

phpBB/styles/prosilver/template/mcp_post.html

 			<p class="post-notice reported">
-				<i class="icon fa-exclamation fa-fw icon-red" aria-hidden="true"></i><span class="sr-only">{{ lang('TOPIC_REPORTED') }}</span> <a href="{{ U_MCP_REPORT }}"><strong>{{ lang('POST_REPORTED') }}</strong></a>
+				{{ Icon('iconify', 'fa:exclamation', lang('TOPIC_REPORTED'), true, 'c-topic-reported-icon') }}
+				<a href="{U_MCP_REPORT}"><strong>{L_MESSAGE_REPORTED}</strong></a>


You changed the used language string here, which is not available.

Suggested change

<a href="{U_MCP_REPORT}">{L_MESSAGE_REPORTED}</a>

<a href="{{ U_MCP_REPORT }}">{{ lang('POST_REPORTED') }}</a>

ghost · 2020-05-08T13:58:46Z

phpBB/styles/prosilver/template/mcp_forum.html

-					<!-- IF topicrow.S_UNREAD_TOPIC --><a href="{topicrow.U_NEWEST_POST}" class="row-item-link"></a><!-- ENDIF -->
+			<dl class="row-item">
+				<dt>
+					<a class="row-item-link{% if topicrow.S_UNREAD_TOPIC %} unread{% else %} read{% endif %}" href="{topicrow.U_NEWEST_POST}" title="{topicrow.TOPIC_FOLDER_IMG_ALT}">


Just noticed this and I am not sure if this applies to all other cases where you create Topic icons.
But there are a couple of issues here.
topicrow.TOPIC_FOLDER_IMG_ALT is not defined, which results in an empty title attribute, which isn't that bad.
topicrow.U_NEWEST_POST is not defined, so making the icon a link with an empty href attribute. topicrow.U_VIEW_TOPIC should be used instead.

yeah i just used the same code as viewforum everywhere seems we need to add that template variable there as it works everywhere else

ghost · 2020-05-08T14:00:33Z

phpBB/styles/prosilver/template/forumlist_body.html

 								<a href="{forumrow.subforum.U_SUBFORUM}" class="subforum<!-- IF forumrow.subforum.S_UNREAD --> unread<!-- ELSE --> read<!-- ENDIF -->" title="<!-- IF forumrow.subforum.S_UNREAD -->{L_UNREAD_POSTS}<!-- ELSE -->{L_NO_UNREAD_POSTS}<!-- ENDIF -->">
-									<i class="icon <!-- IF forumrow.subforum.IS_LINK -->fa-external-link<!-- ELSE -->fa-file-o<!-- ENDIF --> fa-fw <!-- IF forumrow.subforum.S_UNREAD --> icon-red<!-- ELSE --> icon-blue<!-- ENDIF --> icon-md" aria-hidden="true"></i>{forumrow.subforum.SUBFORUM_NAME}</a><!-- IF not forumrow.subforum.S_LAST_ROW -->{L_COMMA_SEPARATOR}<!-- ENDIF -->
+									{{ Icon('iconify', {
+										'fa:external-link' : forumrow.subforum.IS_LINK ,


There's a excessive space between IS_LINK and ,.

Suggested change

'fa:external-link' : forumrow.subforum.IS_LINK ,

'fa:external-link' : forumrow.subforum.IS_LINK,

ghost

One small change and then I think it's done.

ghost · 2020-05-10T10:38:16Z

phpBB/styles/prosilver/template/search_results.html

+								<!-- IF searchresults.ATTACH_ICON_IMG -->{{ Icon('iconify', 'fa:paperclip', '', true) }}<!-- ENDIF -->
 								{% EVENT topiclist_row_topic_by_author_before %}
-								{L_POST_BY_AUTHOR} <!-- EVENT search_results_topic_author_username_prepend -->{searchresults.TOPIC_AUTHOR_FULL}<!-- EVENT search_results_topic_author_username_append --> &raquo; <time datetime="{searchresults.FIRST_POST_TIME_RFC3339}">{searchresults.FIRST_POST_TIME}</time> &raquo; {L_IN} <a href="{searchresults.U_VIEW_FORUM}">{searchresults.FORUM_TITLE}</a>
+								{L_POST_BY_AUTHOR} <!-- EVENT search_results_topic_author_username_prepend -->{searchresults.TOPIC_AUTHOR_FULL}<!-- EVENT search_results_topic_author_username_append --> <time datetime="{searchresults.FIRST_POST_TIME_RFC3339}">{searchresults.FIRST_POST_TIME}</time> &raquo; {searchresults.FIRST_POST_TIME} &raquo; {L_IN} <a href="{searchresults.U_VIEW_FORUM}">{searchresults.FORUM_TITLE}</a>


You now have the time duplicated here.
<time> still shows the text as normal, but allows defining the datetime="" attribute, which is helpful for screenreaders.

weird i literally copy and pasted that whole line

phpBB/includes/functions.php

PHPBB3-16237

hanakin · 2020-08-20T07:57:04Z

@marc1706 seems icons.css was included twice leading to the logo hiccup, also noticed blockquote icon got foobared as well during the rebase so I fixed that as well. Everything should be good now I think

marc1706 · 2020-08-20T08:55:21Z

Will have another look today

marc1706 · 2020-08-21T06:04:50Z

Looks good now. Do you want to fix the invalid commit message that travis complains about yourself or should I do that for you?

hanakin · 2020-08-21T06:12:52Z

might be easier for you to fix if you know whats wrong as i can not see whats wrong with it?

PHPBB3-16237

marc1706 · 2020-08-21T10:01:58Z

Issue was that commit message has an invalid ticket id: PHPBB-16237
Should be: PHPBB3-16237

hanakin added WIP 🚧 4.0 (Triton) 🔮 Chameleon 🦄 New Default Theme labels Nov 30, 2019

hanakin added this to the 4.0.0-a1 milestone Nov 30, 2019

ghost reviewed Dec 3, 2019

View reviewed changes

hanakin removed the WIP 🚧 label Dec 29, 2019

hanakin mentioned this pull request Mar 24, 2020

[ticket/16413] Add an option to define CPF FontAwesome contact icon #5921

Open

4 tasks

hanakin added HTML📝 CSS 📐 JS ☕ php 🐘 labels Apr 11, 2020

hanakin requested a review from a user April 11, 2020 05:08

hanakin added the dependencies Pull requests that update a dependency file label Apr 11, 2020

Crizz0 mentioned this pull request Apr 15, 2020

[ticket/15505] Change approve icon to circle-o and color orange #5115

Closed

4 tasks

hanakin removed the dependencies Pull requests that update a dependency file label Apr 15, 2020

hanakin closed this Apr 15, 2020

hanakin reopened this Apr 15, 2020

hanakin self-assigned this Apr 16, 2020

ghost previously requested changes May 3, 2020

View reviewed changes

ghost reviewed May 4, 2020

View reviewed changes

phpBB/styles/prosilver/template/search_results.html Show resolved Hide resolved

ghost reviewed May 5, 2020

View reviewed changes

ghost reviewed May 7, 2020

View reviewed changes

phpBB/phpbb/template/twig/extension/icon.php Outdated Show resolved Hide resolved

ghost reviewed May 7, 2020

View reviewed changes

ghost suggested changes May 8, 2020

View reviewed changes

ghost suggested changes May 10, 2020

View reviewed changes

ghost approved these changes May 10, 2020

View reviewed changes

hanakin mentioned this pull request May 12, 2020

[ticket/16477] Restore .icon class selector in icons.css #5977

Merged

4 tasks

marc1706 reviewed Aug 12, 2020

View reviewed changes

phpBB/includes/functions.php Outdated Show resolved Hide resolved

hanakin added 20 commits August 19, 2020 10:40

[ticket/16237] Fix icon test

20fe183

PHPBB3-16237

[ticket/16237] use twig 4 responsive post buttons

6d7f535

PHPBB3-16237

[ticket/16237] remove fontawsome tests

277a5b4

PHPBB3-16237

[ticket/16237] Remove responsive menu js stuff

46c6fab

PHPBB3-16237

[ticket/16237] fix minor typos

dd25ed1

PHPBB3-16237

[ticket/16237] rework navbar-footer css only no js

21bef05

PHPBB3-16237

[ticket/16237] need to update WebDriver/phantomJS

3c7d941

PHPBB3-16237

[ticket/16237] fix responsive footer/header navs

98e6006

PHPBB3-16237

[ticket/16237] Add missing template vars

63c7488

PHPBB3-16237

[ticket/16237] cleanup leftovers

50faa5a

PHPBB3-16237

[ticket/16237] fix mcp_forum

a20de9f

PHPBB3-16237

[ticket/16237] fix responsive menu in footer

6badd3e

PHPBB3-16237

[ticket/16237] fix font awsome icon alignment

3eb192a

PHPBB3-16237

[ticket/16237] fix mcp_post

a1b88e1

PHPBB3-16237

[ticket/16237] fix merge conflicts

f7dfff7

PHPBB3-16237

[ticket/16237] Add flash back for now

e904ac8

PHPBB3-16237

[ticket/16237] fix typo

f93f29a

PHPBB3-16237

[ticket/16237] fix typo

e922779

PHPBB3-16237

[ticket/16237] fix search_results

6af33f9

PHPBB3-16237

[ticket/16237] move color from functions

bc59782

PHPBB3-16237

hanakin and others added 4 commits August 21, 2020 12:01

[ticket/16237] fix missed report icon

401eac0

PHPBB3-16237

[ticket/16237] Fix typo in SITE_DESCRIPTION twig syntax

22b03d9

PHPBB3-16237

[ticket/16237] fix quote icon

a9b38bb

PHPBB3-16237

[ticket/16237] include icon.css only once

a060e4f

PHPBB3-16237

marc1706 merged commit 49dd279 into phpbb:master Aug 21, 2020

	<!-- IF not U_SITE_HOME -->{{ Icon('iconify', 'fa:home', lang(L_INDEX), false) }}<!-- ENDIF -->
	{% if U_SITE_HOME %}<span>{{ lang('INDEX') }}</span>{% else %}{{ Icon('iconify', 'fa:home', lang('INDEX'), false) }}{% endif %}

	{{ Icon('iconify', 'fa:angle'-S_CONTENT_FLOW_END, lang('JUMP_TO_POST'), false, 'c-return-arrow-icon') }}
	{{ Icon('iconify', 'fa:angle-' ~ S_CONTENT_FLOW_END, lang('JUMP_TO_POST'), false, 'c-return-arrow-icon') }}

	<a href="{U_MCP_REPORT}"><strong>{L_MESSAGE_REPORTED}</strong></a>
	<a href="{{ U_MCP_REPORT }}"><strong>{{ lang('POST_REPORTED') }}</strong></a>

	'fa:external-link' : forumrow.subforum.IS_LINK ,
	'fa:external-link' : forumrow.subforum.IS_LINK,

Uh oh!

Conversation

hanakin commented Nov 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hanakin commented Nov 30, 2019 •

edited

Loading

hanakin May 8, 2020 •

edited

Loading

ghost May 10, 2020 •

edited by ghost

Loading