feat: Add support to create and build skins | Issue #66 #65

carloslancha · 2020-06-11T20:46:55Z

This pr is, of course, WIP, but I need your feedback to see if this is the right direction (or the one we which we want to go). #66

Purpose of the PR:

Add support to create and build custom skins so we don't need to place all the custom styling in portal, plus we need to change some icons and that only can be done via custom skins.

Creating skins

CKEditor recommends to use one of their base skins to create custom skins so the idea is to be able to choose which one we want to use on the creation moment.

The creation process is pretty simple, we just select the base skin, copy it to our skins folder with the new given name and replace the old name with the new one inside all the files.

Building skins

Following CKEditor docs we need to use their ckbuilder to build the skin (the java one, there's no support for skins on the .sh, maybe we can send them a pr to add it).

For now I just added a script to build the selected skin and place the result inside the ckeditor dist folder. Probably we want to include this as part of the entire build process, but I need some conversation here so we can find the best way to proceed.

…Issue #66

wincent

This approach seems reasonable to me. My only question:

Add support to create and build custom skins

How many of these are we planning on making?

Probably we want to include this as part of the entire build process

Yes, and I imagine, commit the resulting skin files to the repo as well?

wincent · 2020-06-12T11:58:18Z

ck.sh

@@ -85,6 +85,53 @@ case "$COMMAND" in
 		esac
 		;;

+	buildskin)
+		cd skins


This will blow up if you run it before running createskin for the first time.

Rather than mkdir -p skins below, we could just create that directly in the repo and commit it (ie. empty dir with a .gitignore in it so that Git can actually track it) — once we have a skin in here we can remove the .gitignore.

wincent · 2020-06-12T12:00:34Z

ck.sh

+
+		select skin in *
+		do
+			echo "you selected $skin"


AFAIK, if I select a bogus entry, $skin will be an unset or an empty string which means that we'll ed up running:

java -jar dev/builder/ckbuilder/2.3.2/ckbuilder.jar --build-skin ../skins/ ../ckeditor/skins/

So we probably need a check for valid input before proceeding.

but here we're selecting a skin from a list, how a bogus entry could be selected?

said nothing

You can type random crap in, as you've probably discovered.

wincent · 2020-06-12T12:02:23Z

ck.sh

+	createskin)
+		read -r -p "What is the name of the new skin? " skinName
+
+		echo "Which skin do you want to use as base?"


Do we need to offer this much flexibility? Won't we be building exactly one skin from one base? If this is not something that we are going to do often, then we might be able to simplify it.

Probably we don't need it, I need to keep working on not doing more than we need 😅 I'll get rid of it.

To help you, you can print these out and sleep with them under your pillow:

https://en.wikipedia.org/wiki/You_aren%27t_gonna_need_it

https://martinfowler.com/bliki/Yagni.html

https://wiki.c2.com/?YouArentGonnaNeedIt

Alternatively, you could read them. Another option is starting on wiki.c2.com and spending the next 10 hours following links. 😂

wincent · 2020-06-12T12:03:04Z

ck.sh

+			mkdir -p skins
+
+			# Create folder for the new skin
+			mkdir skins/"$skinName"


This is in part what -p is for. Rather than invoking mkdir twice:

-p Create intermediate directories as required. If this option is not specified, the full path prefix of each operand must already exist. On the other hand, with this option specified, no error will be reported if a directory given as an operand already exists. Intermediate directories are created with permission bits of rwxrwxrwx (0777) as modified by the current umask, plus write and search permission for the owner.

…lt skin before building again

carloslancha · 2020-06-15T15:46:30Z

@wincent pr updated

wincent

Seems reasonable to me. You should probably also update the usage() function in the script, and the README also describes what the subcommands are for and how to use them, so should probably update that too.

wincent · 2020-06-15T16:18:04Z

ck.sh

+		do
+			if [[ $skin = "" ]]; then
+				echo "selected skin doesn't exist"
+				break;


carloslancha · 2020-06-15T16:45:14Z

Oooki doki

carloslancha · 2020-06-16T10:07:19Z

@wincent anything else? :P

julien · 2020-06-17T08:27:48Z

@carloslancha I know this is not automated yet, but I'd suggest running shellcheck on the script just to see if there are any errors - if @wincent thinks it's also a good idea

carloslancha · 2020-06-17T14:15:04Z

While trying to build ckeditor to test the new skin I discovered that the skins has to be placed in ckeditor-dev/skins not in ckeditor/skins to make build work. So probably having a task to build only the skin makes no sense, I'm moving it to the build task.

jbalsas · 2020-06-17T14:30:42Z

Once we have our own... I think we probably don't need any other skin, at least it shouldn't be a core feature we provide, to be able to switch skins back to default CKEditor ones.

Just saying, I have no idea if what I said simplifies this or not :D

wincent · 2020-06-17T14:32:35Z

@jbalsas

Once we have our own... I think we probably don't need any other skin, at least it shouldn't be a core feature we provide, to be able to switch skins back to default CKEditor ones.

This is what I was getting at with this comment:

Do we need to offer this much flexibility? Won't we be building exactly one skin from one base? If this is not something that we are going to do often, then we might be able to simplify it.

@carloslancha

While trying to build ckeditor to test the new skin I discovered that the skins has to be placed in ckeditor-dev/skins not in ckeditor/skins to make build work.

Integration testing for the win!

wincent · 2020-06-17T14:34:15Z

carloslancha · 2020-06-17T14:49:39Z

@jbalsas @wincent do you want me to get rid of the createskin script then?

wincent · 2020-06-17T15:16:10Z

It's already there, so leave it. Once the skin is in there, once we can see that it's all working and we don't have to redo things, then we can look at cleaning it up.

carloslancha · 2020-06-17T16:01:27Z

@wincent @jbalsas I've just pushed three commits creating the new skin moono-lexicon (just a copy of moono-lisa for now), setting it as the default skin and moving the skin build process to the general one.

wincent

Two miniscule nits noted inline, but you should feel free to merge this.

wincent · 2020-06-17T17:43:48Z

README.md

+
+1. Create a new skin running `sh ck.sh createskin`.
+2. Edit the skin at `/skins/yourskin` folder.
+3. Build the sking running `sh ck.sh buildskin`.


sking -> skin

wincent · 2020-06-17T17:44:01Z

ck.sh

@@ -2,6 +2,8 @@

 set -e

+CKBUILDER_VERSION="2.3.2"


This seems to do nothing.

…ild process

Carlos Lancha added 2 commits June 11, 2020 22:49

feat: Add support to create new skins based on ckeditor base skins | …

b4a6455

…Issue #66

feat: Add support to build skins | Issue #66

65243dc

carloslancha changed the title ~~Add support to build skins~~ feat: Add support to create and build skins | Issue #66 Jun 11, 2020

wincent reviewed Jun 12, 2020

View reviewed changes

Carlos Lancha added 4 commits June 15, 2020 17:09

feat: create skin folder

6b4e1ab

feat: remove base skin selecction, use moono-lisa as default

b8cffa3

feat: track new and built skin

91563bb

feat: Check if selected skin to build is valid and remove current bui…

fd5e87e

…lt skin before building again

wincent reviewed Jun 15, 2020

View reviewed changes

ck.sh Outdated

do

if [[ $skin = "" ]]; then

echo "selected skin doesn't exist"

break;

Copy link

Contributor

wincent Jun 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No semis.

feat: SF and extract ckbuilder version to var

4c1856c

wincent approved these changes Jun 17, 2020

View reviewed changes

Carlos Lancha added 5 commits June 18, 2020 10:42

feat: update usage and README.md

3891300

feat: create new moono-lexicon skin

911eb3c

feat: change ckeditor skin to moono-lexicon

11d0cc3

feat: remove buildskin script and include that step on the general bu…

52794df

…ild process

docs: added a note to the "testing in liferay" process

b82c14c

carloslancha merged commit bb5c1bb into liferay:master Jun 18, 2020

carloslancha mentioned this pull request Jun 18, 2020

Shellcheck automation #69

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add support to create and build skins | Issue #66 #65

feat: Add support to create and build skins | Issue #66 #65

carloslancha commented Jun 11, 2020 •

edited

Loading

wincent left a comment

wincent Jun 12, 2020

carloslancha Jun 15, 2020

wincent Jun 12, 2020

carloslancha Jun 15, 2020

carloslancha Jun 15, 2020

carloslancha Jun 15, 2020

wincent Jun 15, 2020

carloslancha Jun 15, 2020

wincent Jun 12, 2020

carloslancha Jun 15, 2020

wincent Jun 15, 2020

wincent Jun 12, 2020

carloslancha commented Jun 15, 2020

wincent left a comment

wincent Jun 15, 2020

carloslancha commented Jun 15, 2020

carloslancha commented Jun 16, 2020

julien commented Jun 17, 2020

carloslancha commented Jun 17, 2020 •

edited

Loading

jbalsas commented Jun 17, 2020 •

edited

Loading

wincent commented Jun 17, 2020

wincent commented Jun 17, 2020

carloslancha commented Jun 17, 2020

wincent commented Jun 17, 2020

carloslancha commented Jun 17, 2020

wincent left a comment

wincent Jun 17, 2020

wincent Jun 17, 2020

feat: Add support to create and build skins | Issue #66 #65

feat: Add support to create and build skins | Issue #66 #65

Conversation

carloslancha commented Jun 11, 2020 • edited Loading

Purpose of the PR:

Creating skins

Building skins

wincent left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carloslancha commented Jun 15, 2020

wincent left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carloslancha commented Jun 15, 2020

carloslancha commented Jun 16, 2020

julien commented Jun 17, 2020

carloslancha commented Jun 17, 2020 • edited Loading

jbalsas commented Jun 17, 2020 • edited Loading

wincent commented Jun 17, 2020

wincent commented Jun 17, 2020

carloslancha commented Jun 17, 2020

wincent commented Jun 17, 2020

carloslancha commented Jun 17, 2020

wincent left a 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 11, 2020 •

edited

Loading

carloslancha commented Jun 17, 2020 •

edited

Loading

jbalsas commented Jun 17, 2020 •

edited

Loading