-
Notifications
You must be signed in to change notification settings - Fork 49
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add custom icons to moono-lexicon #70
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is WIP. If you're fine with this approach I'll add the rest of the icons to finish de issue.
Approach makes sense to me.
local OUTPUT | ||
OUTPUT=$(mktemp) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a shellcheck thing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep
ck.sh
Outdated
cp -r skins/moono-lexicon ckeditor-dev/skins/moono-lexicon | ||
|
||
# Convert svg icons to png | ||
node svg-to-png.js |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd move this file out of the repo root, into support/
(just to reduce top-level clutter).
I'd also name it with camelCase, to match the other script in there (checkSubmodule.js
). (In case you're wondering, build-config.js
is the name of the config file you get from the CKEditor website when you download a config, so we didn't change that one.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Roger Roger
ck.sh
Outdated
@@ -149,7 +153,7 @@ case "$COMMAND" in | |||
echo | |||
echo "This will reset the \"patches\" directory and replace these patches:" | |||
echo | |||
ls ../patches/*.patch | cat | |||
find ../patches/*.patch | cat |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shellcheck again?
The thing about this is you still have glob expansion going on, so both versions of this command get expanded as though you had typed (wrapped for readability):
ls ../patches/0001-LPS-89596-Cannot-Drag-Image-from-top-content-line-in.patch \
../patches/0002-LPS-95472-Tabs-in-popups-not-appears-correctly-in-ma.patch \
../patches/0003-LPS-85326-Remove-check-for-Webkit-browsers.patch \
../patches/0004-LPP-36989-Remove-obsolete-summary-field-from-table-e.patch | cat
or:
find ../patches/0001-LPS-89596-Cannot-Drag-Image-from-top-content-line-in.patch \
../patches/0002-LPS-95472-Tabs-in-popups-not-appears-correctly-in-ma.patch \
../patches/0003-LPS-85326-Remove-check-for-Webkit-browsers.patch \
../patches/0004-LPP-36989-Remove-obsolete-summary-field-from-table-e.patch | cat
Now, I imagine that the intent of the shellcheck advice is actually to avoid shell expansion, not to avoid ls
as such... so if you want to do that, the command would be:
find ../patches -name '*.patch'
- No glob expansion, because of the single quotes.
- No useless
cat
(find
, unlikels
, is always going to print one result per line, so it never neededcat
; the purpose of it inls | cat
is to force one item per line).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shellcheck, yep
package.json
Outdated
@@ -20,5 +20,8 @@ | |||
"postversion": "npx liferay-js-publish", | |||
"format": "prettier --write \"skins/**/*.css\" \"support/*.js\" \"*.json\" \"*.md\"", | |||
"format:check": "prettier --list-different \"support/*.js\" \"*.json\" \"*.md\"" | |||
}, | |||
"dependencies": { | |||
"svg-to-png": "^4.0.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bit worrying that this package was last updated 2 years ago.
In an empty project, adding svg-to-png
downloads 55 MB of stuff:
$ du -sh .
55M .
43 MB of that is from phantomjs-prebuilt, which is deprecated:
$ du -sk * | sort -n -r | head -1
44332 phantomjs-prebuilt
But at least yarn audit
is clean... for now...
$ yarn audit
yarn audit v1.22.4
0 vulnerabilities found - Packages audited: 219
✨ Done in 0.51s.
I haven't done an exhaustive search, but I did find svgexport on Stack Overflow. It has a bit over twice as many weekly downloads (7.6k vs 3.1k), and has been updated more recently (5 months ago).
Installing it pulls down 281 MB of crap:
$ du -sh .
281M .
But 278 MB of that is puppeteer:
$ cd node_modules
$ du -sk * | sort -n -r | head -1
284600 puppeteer
And it also has a clean bill of yarn audit
health:
$ yarn audit
yarn audit v1.22.4
0 vulnerabilities found - Packages audited: 46
✨ Done in 0.97s.
Just a quick skim over the source, it does look a bit more minimal/modern. I'd say it's a better choice than svg-to-png
, but it might be worth a quick survey to see if there's nothing better out there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll take a look
svg-to-png.js
Outdated
console.log('PNG icons created'); | ||
}; | ||
|
||
convert(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You shouldn't fire off an async function and then just walk away. Instead, something like this:
convert()
.catch(error => {
console.log(error);
process.exit(1);
});
All of this to avoid an "unhandled promise rejection" error.
Once we have top-level await broadly available, this will be nicer:
try {
await convert();
} catch (error) {
console.log(error);
process.exit(1);
}
skins/moono-lexicon/icons/italic.svg
Outdated
<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 512 512"> | ||
<path class="lexicon-icon-outline italic-i-stem" d="M165.23 512h92.562l60.343-342.235h-92.526z"></path> | ||
<path class="lexicon-icon-outline italic-i-dot" d="M330.423 16.713c-10.935-11.154-24.503-16.713-40.704-16.713-15.762 0-29.221 5.559-40.338 16.713-11.154 11.118-16.713 24.832-16.713 41.033 0 15.323 5.559 28.489 16.713 39.643 11.118 11.118 24.576 16.713 40.338 16.713 16.201 0 29.769-5.559 40.704-16.713 10.862-11.154 16.347-24.357 16.347-39.643 0-16.238-5.486-29.915-16.347-41.033z"></path> | ||
</svg> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sucks that we'll have to keep these in sync manually, but at least you're making a nice script to automate the build
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not add a dependency on @clayui/css
? Don't we publish these in images/icons
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea.
└── @clayui
└── css
├── CHANGELOG.md
├── LICENSE.txt
├── README.md
├── index.js
├── lib
│ ├── css
│ │ ├── atlas.css
│ │ ├── atlas.css.map
│ │ ├── base.css
│ │ ├── base.css.map
│ │ ├── bootstrap.css
│ │ └── bootstrap.css.map
│ ├── images
│ │ └── icons
│ │ ├── add-cell.svg
│ │ ├── add-column.svg
│ │ ├── add-role.svg
│ │ ├── add-row.svg
│ │ ├── adjust.svg
│ │ ├── ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, I was thinking about adding it as a dependency. Wondering if we can take advantage of it and use scss variables so we don't need to hardcode'em...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds exactly like the type of work for a build script 😂
I wouldn't mind hardcoding them temporarily and see where that lead us
So... how does this look? Is it already applied? Can you share a screenshot for our delight? :D |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks fine to me. Some nits inline.
"output": "align-image-right" | ||
} | ||
] | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be a bit prettier as:
{
"dir": "../../../node_modules/@clayui/css/lib/images/icons",
"icons": {
"align-center": "justifycenter",
"align-image-center": "align-image-center",
"align-image-left": "align-image-left",
"align-image-right": "align-image-right",
"align-justify": "justifyblock",
"align-left": "justifyleft",
"align-right": "justifyright",
"bold": "bold",
"chain-broken": "unlink",
"change-list": "replace",
"code": "source",
"cut": "cut",
"flag-full": "anchor",
"full-size": "maximize",
"hidden": "hidden",
"indent-less": "outdent",
"indent-more": "indent",
"italic": "italic",
"link": "link",
"list-ol": "numberedlist",
"list-ul": "bulletedlist",
"paste": "copy",
"picture": "image",
"quote-right": "blockquote",
"redo": "redo",
"remove-style": "removeformat",
"search": "find",
"separator": "horizontalrule",
"strikethrough": "strike",
"subscript": "subscript",
"superscript": "superscript",
"table": "table",
"underline": "underline",
"undo": "undo"
}
}
support/svgToPng.js
Outdated
|
||
const sourceIconsPath = path.join(path.dirname(configFilePath), '../', iconsConfig.dir) | ||
|
||
for (const icon of iconsConfig.icons) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you change the JSON, here would be:
for (const [source, output] of Object.entries(iconsConfig.icons)) {
support/svgToPng.js
Outdated
let rawdata = fs.readFileSync(configFilePath) | ||
let iconsConfig = JSON.parse(rawdata) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const
, not let
.
Also, rawdata
→ rawData
support/svgToPng.js
Outdated
let rawdata = fs.readFileSync(configFilePath) | ||
let iconsConfig = JSON.parse(rawdata) | ||
|
||
const sourceIconsPath = path.join(path.dirname(configFilePath), '../', iconsConfig.dir) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The whole point of join
is that you don't have to specify path separators, so '../'
→ '..'
.
support/svgToPng.js
Outdated
const fs = require('fs') | ||
const path = require( 'path' ) | ||
const sharp = require("sharp") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like you need to run Prettier over this file (missing semi-colons, below too; inconsistent bracket spacing, mixed quotes; indents off below).
@wincent @jbalsas I just pushed what I had for you to see the approach, I was saturated (don't know if that's the correct word but 🤷), didn't format and that stuff, sorry for not specifying. I'll add an screenshot tomorrow, it's working, just need to review if the color is the correct one (and fix some icons that are not ok in clay, but that's another task for them). |
We're now using sharp for png conversion:
This is how it looks in portal As you can see there some of things we need to take care about: Icons sizeIn Clay all svg icons are 512x512, but those dimensions are supposed to refer to the viewbox, not to the icon itself. For example, the Bold icon, is not supposed to fill the entire viewbox (if we resize it to 16x16 the icon height should be 11.98px). That's why some icons looks that big. Custom pluginsCustom plugins like image selector defines their icon in portal:
so we need to change those icons in portal. Missing iconsThere're some icons that are not ready yet (listed here). That's why some of the buttons appear with default ckeditor ones. Once Lexicon takes care of it and Clay add it to the library we'll just need to add them to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lg2m
skins/moono-lexicon/icons/icons.json
Outdated
"bold": "bold", | ||
"italic": "italic", | ||
"underline": "underline", | ||
"strikethrough": "strike", | ||
"subscript": "subscript", | ||
"superscript": "superscript", | ||
"remove-style": "removeformat", | ||
"list-ol": "numberedlist", | ||
"list-ul": "bulletedlist", | ||
"indent-more": "indent", | ||
"indent-less": "outdent", | ||
"quote-right": "blockquote", | ||
"align-left": "justifyleft", | ||
"align-center": "justifycenter", | ||
"align-right": "justifyright", | ||
"align-justify": "justifyblock", | ||
"link": "link", | ||
"chain-broken": "unlink", | ||
"flag-full": "anchor", | ||
"picture": "image", | ||
"table2": "table", | ||
"separator": "horizontalrule", | ||
"full-size": "maximize", | ||
"change-list": "replace", | ||
"search": "find", | ||
"undo": "undo", | ||
"redo": "redo", | ||
"paste": "copy", | ||
"cut": "cut", | ||
"code": "source", | ||
"hidden": "hidden", | ||
"align-image-center": "align-image-center", | ||
"align-image-left": "align-image-left", | ||
"align-image-right": "align-image-right" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
support/svgToPng.js
Outdated
for (const [source, output] of Object.entries(iconsConfig.icons)) { | ||
var svgData = fs.readFileSync(`${sourceIconsPath}/${source}.svg`, 'utf8'); | ||
|
||
svgData = svgData.replace(/\<svg/g, '<svg fill="#6B6C7E"'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the only brittle part of this whole business. What if the Clay SVGs ever start including a fill
attribute? You'd get two fill
attributes then (at the moment, looks like only the "flags" ones do, for obvious reasons.) Why do you use g
here? I'd expect one svg per file (looking at the Clay SVGs, that's the case for all the ones that we care about here).
Feel free to ship this as-is, but if you want to be a little tighter you could do something like:
svgData = svgData
.replace(/\s+fill="[^"]+"/, '')
.replace(/<svg/, '<svg fill="#6b6c7e"');
(ie. strip fill
attribute, if any, then add new attribute). I probably wouldn't bother, but mentioning it just for completeness.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Roger
I presume you're going to adjust in some way? |
Nope, I can't, that's something that need to be fixed in Clay. |
What would be fixed there? I read you description but I don't get exactly what is wrong and what would be needed. Why does it only affect some icons? Do we have a Clay ticket for the change that you would need to be made there? And are you sure that change won't break something else elsewhere?
You sure? You have a script where you can run arbitrary transformations. |
I'll try to explain it better. All Clay icons are made in 512x512px (resized to 32x32 or 16x16) viewbox base, but that doesn't mean that the icons must fill the entire available space. For example, this is the Lexicon definition of the As you can see the viewbox is 16x16, but the icon doesn't fill the entire container. This is the definition of the In this case the viewbox is the same, 16x16, but the icon is bigger than the bold one. This is to control the visual weights of the icons, each one has its own size inside the same viewbox. Now, this is the svg of the it fills the entire space, so we need to fix the icons that are not following the definition.
@drakonux was going to move this, but if he's no time tomorrow I can help and start opening thickets in Clay. |
…pi folder to upload it, sharp needs this folder to generate the icons inside
I was checking the CKEditor 4 demo and was wondering if we could do something like:
It looks like we can overwrite the moono-lisa CSS. If you need the SVG's converted to data uris just give me a list and colors you want for each. Edit: The good thing about this pattern is you can adjust the icon size without having to generate a whole new set of PNG's. If bold is too big:
|
Changes lg2m. So based on Slack, my understanding is that we're going to:
|
https://issues.liferay.com/browse/IFI-1775
AFAIK moono-lisa (base skin for moono-lexicon) only supports PNG icons but all our clay icons are SVG, so with this pr I'm introducing a step in the build process to convert those SVG into 16px and 32px PNG.
CKEditorBuilder get all that individual icon files and creates a PNG sprite.
This is WIP. If you're fine with this approach I'll add the rest of the icons to finish de issue.