feat: add support for svg icons #74

carloslancha · 2020-06-25T17:33:21Z

https://issues.liferay.com/browse/IFI-1829

We need to use SVG icons as we discussed on slack

In this pr we're generating needed icon classes for all the different states and adding required support for rtl and ltr.

Striked out ckeditor icons:

jbalsas · 2020-06-25T17:38:17Z

skins/moono-lexicon/icons/icons.json

+            "rtr": "indent-more"
+        },
+        "redo": {
+            "ltr": "redo",


Are you sure these are directional? It kinda makes sense... but I'd love to get some reading about it :D

CKEditor have it as directional (automatically uses .cke-ltr or .cke-rtl classes`). I assumed it was correct.

jbalsas · 2020-06-25T17:39:01Z

skins/moono-lexicon/icons/icons.json

-        "find": "search",
+        "find": {
+            "ltr": "search",
+            "rtl": "search"


Is this mean to be a different search? Or if they're the same, why separate rtl and ltr?

Same, CKEditor have it as directional so we need to specify the icon for both directions. Probably we'll need to specify other icon, something like search-rtl

jbalsas · 2020-06-25T17:39:18Z

skins/moono-lexicon/icons/icons.json

-        "anchor": "flag-full",
+        "anchor": {
+            "ltr": "flag-full"
+        },
        "blockquote": "quote-right",


Shouldn't this one be directional as well? :D

It is but we don't have the rtl icon yet. I can use the same until we have it or just leave it blank.

jbalsas · 2020-06-25T17:39:49Z

skins/moono-lexicon/icons/icons.json

@@ -4,36 +4,59 @@
        "align-image-center": "align-image-center",
        "align-image-left": "align-image-left",
        "align-image-right": "align-image-right",
-        "anchor": "flag-full",
+        "anchor": {
+            "ltr": "flag-full"


Same here... why just ltr?

I just have no idea what this is supposed to do, it just feels weird :D

Same as the others

jbalsas · 2020-06-25T17:41:01Z

support/iconsClassesGenerator.js

+		}`;
+
+		iconsCSSContent += `${activeIconCSS} ${defaultIconCSS} ${disableIconCSS} ${hoverIconCSS} ${focusIconCSS}`;
+	*/


Are we still saving all these comments? :D

Not sure if this is something you want to keep... but if you do, one option is to create a bogus commit adding it and one removing it, so you will have it in the git history :D

Ups, no, I have it already in the history, just forgot to remove it, thx

jbalsas · 2020-06-25T17:41:32Z

ck.sh

@@ -59,7 +59,7 @@ case "$COMMAND" in
 				cd ckeditor-dev

 				# Convert svg icons to png


I assume this comment is no longer acurate?

carloslancha · 2020-06-26T06:57:38Z

About rtl icons:

CKEditor automatically adds to the css the classes for all the icons in ltr (just using .cke_button .cke_button__name_icon) and some for rtl(adding .cke_rtl .cke_button .cke_button_name_icon).

There are some of those rlt icons that maybe we don't want to mirror. Material Design for example has some guidelines about it.

Search icon: Lens handle is usually at the right side because majority of users are right-handed, also in RTL-writing countries.
Undo / Redo: These icons refers to time, clockwise (right -> redo) and counterclockwise (left > undo). In Google's case they use a circular icon that could be associated better with the clock, our icons are not exactly circular but I think that still could be associated in the same way.
Anchor / Flag: Nothing about this but I guess we can follow the same principle as the search icon. (Already listed as rtl in our icons excel)

Anyway, not an expert, maybe @drakonux can provide more info :)

carloslancha · 2020-06-26T06:59:40Z

Forgot to mention that even if we don't want to mirror some of those icons we need to specify the rtl icon using the same one as ltr just because CKEditor works in that way.

jbalsas · 2020-06-26T07:04:57Z

LGTM, @carloslancha!

Just one question... could you highlight which of the icons in the gif are ours and which ones are ckeditor's?

carloslancha · 2020-06-26T08:36:37Z

@jbalsas updated description

wincent · 2020-06-26T09:16:45Z

support/iconsClassesGenerator.js

+	const activeIconCSS = `${directionClass}.cke_hidpi .cke_button.cke_button_on .cke_button__${cKEditorIcon}_icon,
+		${directionClass} .cke_button.cke_button_on .cke_button__${cKEditorIcon}_icon {
+			background: url("data:image/svg+xml;charset=utf8,${encodeSvgData(
+				svgData,
+				activeColor
+			)}") !important;
+		}`;


nit: I'd format these so that the output is consistently indented:

const activeIconCSS = ` ${directionClass}.cke_hidpi .cke_button.cke_button_on .cke_button__${cKEditorIcon}_icon, ${directionClass} .cke_button.cke_button_on .cke_button__${cKEditorIcon}_icon { background: url("data:image/svg+xml;charset=utf8,${encodeSvgData( svgData, activeColor )}") !important; } `;

(The way you have it, you have 0 indent on first line, then it jumps to two tabs of indent)

wincent · 2020-06-26T09:17:51Z

support/iconsClassesGenerator.js

+			)}") !important;
+		}`;
+
+	return `${activeIconCSS} ${defaultIconCSS} ${disableIconCSS} ${hoverIconCSS} ${focusIconCSS}`;


Maybe?

return [ activeIconCSS, defaultIconCSS, disableIconCSS, hoverIconCSS, focusIconCSS, ].join('\n\n');

wincent · 2020-06-26T09:18:45Z

support/iconsClassesGenerator.js

+		.replace(/(<svg[^>]*)( fill="[^"]+")/, '\1')
+		.replace(/<svg/, `<svg fill="${color}"`)
+		.replace(/"/g, "'") // Use single quotes instead of double to avoid encoding.
+		.replace(/>\s{1,}</g, '><')


{1,} is just +, isn't it? Why not use that?

wincent · 2020-06-26T09:19:46Z

ck.sh

@@ -58,8 +58,8 @@ case "$COMMAND" in

 				cd ckeditor-dev

-				# Convert svg icons to png
-				node ../support/svgToPng.js skins/moono-lexicon/icons/icons.json skins/moono-lexicon/icons
+				# Generate svg icons CSS Classes


nit: Consistent capitalization? (ie. "svg" vs "CSS")

carloslancha · 2020-06-26T09:52:09Z

@wincent formated

Carlos Lancha added 9 commits June 25, 2020 19:33

feat: add base styles for svg icons in moono-lexicon

789c087

feat: add script to generate default css classes for svg icons

239dea4

feat: generate focus, hover, active and disable icon states

03f7eca

chore: refactor

edddb32

fix: fix focus state syles

c7c6a5c

feat: add support for ltr and rtl icons

c81cb21

feat: remove no longer needed files

a0886cf

feat: run iconsClassesGenerator on build

e8b036f

feat: sf

192a1fe

jbalsas reviewed Jun 25, 2020

View reviewed changes

feat: typo

caf9940

carloslancha changed the title ~~feat: sf~~ feat: add support for svg icons Jun 25, 2020

Carlos Lancha added 2 commits June 26, 2020 10:27

feat: add missing rtl icons

abd14ca

feat: remove only svg fill attribute

8119936

wincent reviewed Jun 26, 2020

View reviewed changes

feat: format

3723b95

feat: add imageselector icon

7d214a7

carloslancha merged commit 5b9e146 into liferay:master Jun 29, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add support for svg icons #74

feat: add support for svg icons #74

carloslancha commented Jun 25, 2020 •

edited

Loading

jbalsas Jun 25, 2020

carloslancha Jun 25, 2020

jbalsas Jun 25, 2020

carloslancha Jun 25, 2020

jbalsas Jun 25, 2020

carloslancha Jun 25, 2020

jbalsas Jun 25, 2020

carloslancha Jun 25, 2020

jbalsas Jun 25, 2020

carloslancha Jun 25, 2020

jbalsas Jun 25, 2020

carloslancha Jun 25, 2020

carloslancha commented Jun 26, 2020 •

edited

Loading

carloslancha commented Jun 26, 2020 •

edited

Loading

jbalsas commented Jun 26, 2020

carloslancha commented Jun 26, 2020

wincent Jun 26, 2020

wincent Jun 26, 2020

wincent Jun 26, 2020

wincent Jun 26, 2020

carloslancha commented Jun 26, 2020

		@@ -59,7 +59,7 @@ case "$COMMAND" in
		cd ckeditor-dev

		# Convert svg icons to png

feat: add support for svg icons #74

feat: add support for svg icons #74

Conversation

carloslancha commented Jun 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carloslancha commented Jun 26, 2020 • edited Loading

carloslancha commented Jun 26, 2020 • edited Loading

jbalsas commented Jun 26, 2020

carloslancha commented Jun 26, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carloslancha commented Jun 26, 2020

carloslancha commented Jun 25, 2020 •

edited

Loading

carloslancha commented Jun 26, 2020 •

edited

Loading

carloslancha commented Jun 26, 2020 •

edited

Loading