LPS-116194 Add token-definition part 1 #119

edalgrin · 2020-07-15T08:32:38Z

Work in progress to be merged to #111

liferay-continuous-integration · 2020-07-15T08:32:42Z

To conserve resources, the PR Tester does not automatically run for every pull.

If your code changes were already tested in another pull, reference that pull in this pull so the test results can be analyzed.

If your pull was never tested, comment "ci:test" to run the PR Tester for this pull.

jbalsas · 2020-07-15T09:28:11Z

modules/apps/frontend-theme/frontend-theme-classic/src/WEB-INF/token-definition.json

+	],
+	"tokens": [
+		{
+            "ccp": "--white",


So, this is the only thing... @izaera, @ruben-pulido, @marcoscv-work, @wincent... thoughts on how to better model it now that we can see it?

I still think something like:

{ "editorType": "ColorPicker", "label": "gray-100", "mappings": { "ccp": "gray-100" "figma": { "id": 200, "value": "gray-100", } }, "name": "gray100Color", "tokenCategoryName": "colors", "tokenSetName": "general", "type": "String" }

We could make the CSS CustomProperties mapping as simple as a string to allow for more complex ones in the future.

We would need to decide if we want mappings to be an indexed object as in the example or an array with mappingType and mapping properties. The second one is more verbose but also more readable...

Thoughts?

I prefer indexed objects because:

They enforce keys to be unique

They let you index by key in O(1)

Said that, it is obvious that we can do whatever we want in the JSON and then transform it internally to suit our programming needs.

However, I don't know how easy/possible it is to enforce unique keys inside an array in JSON schema. That could be key...

I was wondering why we include custom mappings in this file:

Because it's their final place and we want them to be here

Because we don't know yet what to do with them and we are just proofing

@ruben-pulido seems to be aligned with me (as per private conversation) and thinks the reason is 2

However, I don't know how easy/possible it is to enforce unique keys inside an array in JSON schema. That could be key...

The current specification of JSON schema does not allow it AFAIK.
There is an open issue asking for this.

In other places in the portal where we want to guarantee uniqueness we are adding code to perform that check outside of the JSON schema validation

it will be way too easy for tokens to fall out of sync and have no mappings

That's a point too 🤔

My concern was that the person writing the tokens may not know anything about the mappings. It could even be that the mappings are not known at the stage where the theme is developed. But it is true that that problem may be fixed with a proper development workflow...

Maybe we can settle on adding mappings in the file and then let it be extended (after deployment of the theme) if needed.

In that case, we just need to decide if we want an array or an object...

Well... seeing as we have a schema and indexed objects are not spec'd... looks like we'll have an array?

{ "editorType": "ColorPicker", "label": "gray-100", "mappings": [ { "mappingType": "cssVariable", "value": "gray-100" } ], "name": "gray100Color", "tokenCategoryName": "colors", "tokenSetName": "general", "type": "String" }

This, @ruben-pulido, @izaera, @eduardoallegrini? 👆

That looks good to me. I'll update the token definition JSON schema in #99 accordingly

"mappings": [
{
"mappingType": "cssVariable",
"value": "gray-100"
}
],

I'll use type instead of mappingType as the name of the field. Prefixing type but not prefixing value seems otherwise inconsistent

The example mentioned above will then be:

{ "editorType": "ColorPicker", "label": "gray-100", "mappings": [ { "type": "cssVariable", "value": "gray-100" } ], "name": "gray100Color", "tokenCategoryName": "colors", "tokenSetName": "general", "type": "String" }

@eduardoallegrini Can you modify token-definition.json to include the change?

edalgrin · 2020-07-15T10:27:41Z

I have a question about the "editorType": "ColorPicker" if we only have one, can't we include it as "type": "Color" and process it with something like this?

if (type === "Color") {
   editor = ColorPicker;
   type = String;
}

ruben-pulido · 2020-07-15T10:34:03Z

modules/apps/frontend-theme/frontend-theme-classic/src/WEB-INF/token-definition.json

+	],
+	"tokens": [
+		{
+            "ccp": "--white",


What does "ccp" stand for?

CSS Custom Properties, it's not the most fancy name but it's the official one 🤷‍♂️ https://www.w3.org/TR/css-variables-1/

I guess CSS Custom Property

edalgrin · 2020-07-15T10:38:56Z

Another question: what are tokenCategoryName and tokenSetName? I tried to give a meaning on my own but it's not really clear to me

izaera · 2020-07-15T10:47:15Z

Other than my feedback in this thread, the rest LGTM.

I would probably rename ccp to something more comprehensible and less similar to CCCP 😅 , but I'm OK with it if you find it correct. For example: css looks OK, since the only mappings from tokens to CSS that come to mind are "CSS custom properties", I think.

izaera · 2020-07-15T11:16:36Z

@eduardoallegrini Regarding your question: editorType is the UI, type is the data type. They are different things.

For example, a variable of type string can be edited with a color picker or an input.

We could even potentially allow more than one editorType if we wanted to...

wincent · 2020-07-15T11:19:50Z

Just to give some feedback from an "outsider" perspective (ie. somebody who doesn't know much about our Custom CSS Properties implementation), this are the questions that I have in my mind as I read this:

How did we decide on the ordering inside tokenCategories, tokenSets and, tokens?
We have both a "general" category and a "general" set, which may be confusing. At the moment in this PR we have tokens using the category/set pairs listed below; so it's a bit confusing to me that we have a "general" set inside the "colors" category, but we also have a "utility" set inside the "general" category... I don't know if this structure is dictated by our existing Custom CSS Properties implementation:
- "colors" category / "general" set
- "colors" category / "utility" set
- "colors" category / "layout" set
- "general" category / "utility" set
- "spaces" category / "layout" set
- "typography" category / "utility" set
- "typography" category / "general" set
I don't know if this is a partial example of that, but there are a number of sets that have no tokens defined for them at all (eg. "alerts", "cards" etc).

wincent · 2020-07-15T11:22:10Z

Oh, and I forgot to mention, "layout" (set) + "colors" (categories) is a bit of a strange combination to me. We're using it in this PR for --body-bg and --body-color. The body doesn't feel like a layout primitive to me, so I'm not sure why we're classifying it this way.

jbalsas · 2020-07-15T11:28:09Z

Ok, quickly from https://www.figma.com/file/R0ccjMTdAmU3t348wSTUUJ/LPS---115168---Style-Editor-02?node-id=30%3A16019

How did we decide on the ordering inside tokenCategories, tokenSets and, tokens?

Arbitrary, however we want... probably through some heuristic based on importance and frequency of usage, but as of right now, just make a guess.

modules/apps/frontend-theme/frontend-theme-classic/src/WEB-INF/token-definition.json

wincent · 2020-07-15T11:43:39Z

Arbitrary, however we want... probably through some heuristic based on importance and frequency of usage, but as of right now, just make a guess.

Awesome, that clears it up perfectly then.

Based on the Figma, then perhaps we have some minor discrepancies in terminology:

Figma category	PR category
General	general
Layout	spaces
Text	typography
Color	colors

and:

Figma set	PR set
Border radius	Utility
etc ...

Not sure what the "source of truth" should be considered to be, but I do know that whenever the language for concepts is not consistent throughout all the levels of a system (eg. UI terminology, terminology at various levels inside the implementation, terminology inside data/configuration artifacts etc) that it can increase the cognitive load of working with the system. It's not always possible, of course, for everything to be in exact harmony, but insofar as it is possible, it is desirable to keep discrepancies to a minimum.

ruben-pulido · 2020-07-15T11:44:33Z

I would probably rename ccp to something more comprehensible and less similar to CCCP 😅 , but I'm OK with it if you find it correct. For example: css looks OK, since the only mappings from tokens to CSS that come to mind are "CSS custom properties", I think.

I agree: ccp is a bit cryptic. I find css or cssCustomProperties better options

edalgrin · 2020-07-15T11:44:57Z

@wincent

How did we decide on the ordering inside tokenCategories, tokenSets and, tokens?

I tried to follow the order of the notes https://www.notion.so/Stylebook-Variables-a919ebecc01046d5bb8f89e076842503

We have both a "general" category and a "general" set, which may be confusing. At the moment in this PR we have tokens using the category/set pairs listed below; so it's a bit confusing to me that we have a "general" set inside the "colors" category, but we also have a "utility" set inside the "general" category... I don't know if this structure is dictated by our existing Custom CSS Properties implementation:

"colors" category / "general" set

"colors" category / "utility" set

"colors" category / "layout" set

"general" category / "utility" set

"spaces" category / "layout" set

"typography" category / "utility" set

"typography" category / "general" set

You are right! I don't know how to define them

I don't know if this is a partial example of that, but there are a number of sets that have no tokens defined for them at all (eg. "alerts", "cards" etc).

I tried to copy the categories we defined in https://github.com/liferay/liferay-portal/blob/master/modules/apps/frontend-theme/frontend-theme-classic/src/css/custom_properties/_custom_properties_variables.scss

jbalsas · 2020-07-15T11:48:13Z

I tried to copy the categories we defined in https://github.com/liferay/liferay-portal/blob/master/modules/apps/frontend-theme/frontend-theme-classic/src/css/custom_properties/_custom_properties_variables.scss

This should be customer-data-ux-driven... so maybe ping the author of https://www.notion.so/Stylebook-Variables-a919ebecc01046d5bb8f89e076842503, Juan? :)

edalgrin · 2020-07-15T12:06:12Z

@izaera @ruben-pulido

I would probably rename ccp to something more comprehensible and less similar to CCCP 😅 , but I'm OK with it if you find it correct. For example: css looks OK, since the only mappings from tokens to CSS that come to mind are "CSS custom properties", I think.

I agree: ccp is a bit cryptic. I find css or cssCustomProperties better options

What about cssVariable as @jbalsas suggested previously?

ruben-pulido · 2020-07-15T12:13:13Z

What about cssVariable as @jbalsas suggested previously?

Sounds OK to me since that seems to be a term commonly used (https://developer.mozilla.org/en-US/docs/Web/CSS/--*)

izaera · 2020-07-15T12:16:32Z

@ruben-pulido Are all these changes included in the JSON schema or it has to be modified?

edalgrin · 2020-07-15T13:13:30Z

I've updated the PR making the changes mentioned in the comments. I didn't modify the categories and sets yet, because we have scheduled a meeting tomorrow to talk about it, but I think @izaera can proceed with the integration.

ruben-pulido · 2020-07-15T13:32:03Z

@ruben-pulido Are all these changes included in the JSON schema or it has to be modified?

@izaera I'm going to modify the JSON schema to include the mapping as discussed here and will link it here once finished

ruben-pulido · 2020-07-15T13:53:36Z

@ruben-pulido Are all these changes included in the JSON schema or it has to be modified?

@izaera I'm going to modify the JSON schema to include the mapping as discussed here and will link it here once finished

Token definition JSON schema updated with mappings:

See liferay-frontend#119 (comment) for more information.

jbalsas · 2020-07-15T14:28:09Z

Closing here since it's been updated there

See liferay-frontend#119 (comment) for more information.

edalgrin marked this pull request as draft July 15, 2020 08:32

jbalsas reviewed Jul 15, 2020

View reviewed changes

ruben-pulido reviewed Jul 15, 2020

View reviewed changes

jbalsas reviewed Jul 15, 2020

View reviewed changes

modules/apps/frontend-theme/frontend-theme-classic/src/WEB-INF/token-definition.json Outdated Show resolved Hide resolved

LPS-116194 Add style-editor token-definition

3d8eecf

edalgrin force-pushed the LPS-116194 branch from 3af8748 to 3d8eecf Compare July 15, 2020 13:05

edalgrin mentioned this pull request Jul 15, 2020

LPS-115484 Token definitions API and implementation #111

Closed

ruben-pulido mentioned this pull request Jul 15, 2020

LPS-116193 - As a developer I can specify custom tokens so that the style editor provides a form to edit those tokens instead of the Liferay-standard ones #99

Closed

izaera added a commit to izaera/liferay-portal that referenced this pull request Jul 15, 2020

LPS-115484 Update token definition schema according to review

2f78d92

See liferay-frontend#119 (comment) for more information.

jbalsas closed this Jul 15, 2020

ruben-pulido mentioned this pull request Jul 16, 2020

LPS-115484 Token definitions API and implementation brianchandotcom/liferay-portal#91387

Closed

izaera added a commit to izaera/liferay-portal that referenced this pull request Jul 17, 2020

LPS-115484 Update token definition schema according to review

942530b

See liferay-frontend#119 (comment) for more information.

jbalsas pushed a commit to izaera/liferay-portal that referenced this pull request Jul 17, 2020

LPS-115484 Update token definition schema according to review

b0a23af

See liferay-frontend#119 (comment) for more information.

ealonso pushed a commit to ealonso/liferay-portal that referenced this pull request Jul 20, 2020

LPS-115484 Update token definition schema according to review

ae78b2b

See liferay-frontend#119 (comment) for more information.

izaera added a commit to izaera/liferay-portal that referenced this pull request Jul 20, 2020

LPS-115484 Update token definition schema according to review

bcce6fb

See liferay-frontend#119 (comment) for more information.

ealonso pushed a commit to ealonso/liferay-portal that referenced this pull request Jul 21, 2020

LPS-115484 Update token definition schema according to review

e0342c0

See liferay-frontend#119 (comment) for more information.

brianchandotcom pushed a commit to brianchandotcom/liferay-portal that referenced this pull request Jul 22, 2020

LPS-115484 Update token definition schema according to review

520c644

See liferay-frontend#119 (comment) for more information.

edalgrin deleted the LPS-116194 branch September 4, 2020 07:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LPS-116194 Add token-definition part 1 #119

LPS-116194 Add token-definition part 1 #119

edalgrin commented Jul 15, 2020 •

edited

Loading

liferay-continuous-integration commented Jul 15, 2020

jbalsas Jul 15, 2020

izaera Jul 15, 2020

izaera Jul 15, 2020

ruben-pulido Jul 15, 2020

ruben-pulido Jul 15, 2020

izaera Jul 15, 2020

jbalsas Jul 15, 2020

ruben-pulido Jul 15, 2020

ruben-pulido Jul 15, 2020

ruben-pulido Jul 15, 2020

edalgrin commented Jul 15, 2020 •

edited

Loading

ruben-pulido Jul 15, 2020

edalgrin Jul 15, 2020 •

edited

Loading

ruben-pulido Jul 15, 2020

edalgrin commented Jul 15, 2020

izaera commented Jul 15, 2020

izaera commented Jul 15, 2020 •

edited

Loading

wincent commented Jul 15, 2020

wincent commented Jul 15, 2020

jbalsas commented Jul 15, 2020

wincent commented Jul 15, 2020

ruben-pulido commented Jul 15, 2020 •

edited

Loading

edalgrin commented Jul 15, 2020 •

edited

Loading

jbalsas commented Jul 15, 2020

edalgrin commented Jul 15, 2020 •

edited

Loading

ruben-pulido commented Jul 15, 2020

izaera commented Jul 15, 2020

edalgrin commented Jul 15, 2020 •

edited

Loading

ruben-pulido commented Jul 15, 2020 •

edited

Loading

ruben-pulido commented Jul 15, 2020

jbalsas commented Jul 15, 2020

LPS-116194 Add token-definition part 1 #119

LPS-116194 Add token-definition part 1 #119

Conversation

edalgrin commented Jul 15, 2020 • edited Loading

liferay-continuous-integration commented Jul 15, 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

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

edalgrin commented Jul 15, 2020 • edited Loading

Choose a reason for hiding this comment

edalgrin Jul 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edalgrin commented Jul 15, 2020

izaera commented Jul 15, 2020

izaera commented Jul 15, 2020 • edited Loading

wincent commented Jul 15, 2020

wincent commented Jul 15, 2020

jbalsas commented Jul 15, 2020

wincent commented Jul 15, 2020

ruben-pulido commented Jul 15, 2020 • edited Loading

edalgrin commented Jul 15, 2020 • edited Loading

jbalsas commented Jul 15, 2020

edalgrin commented Jul 15, 2020 • edited Loading

ruben-pulido commented Jul 15, 2020

izaera commented Jul 15, 2020

edalgrin commented Jul 15, 2020 • edited Loading

ruben-pulido commented Jul 15, 2020 • edited Loading

ruben-pulido commented Jul 15, 2020

jbalsas commented Jul 15, 2020

edalgrin commented Jul 15, 2020 •

edited

Loading

edalgrin commented Jul 15, 2020 •

edited

Loading

edalgrin Jul 15, 2020 •

edited

Loading

izaera commented Jul 15, 2020 •

edited

Loading

ruben-pulido commented Jul 15, 2020 •

edited

Loading

edalgrin commented Jul 15, 2020 •

edited

Loading

edalgrin commented Jul 15, 2020 •

edited

Loading

edalgrin commented Jul 15, 2020 •

edited

Loading

ruben-pulido commented Jul 15, 2020 •

edited

Loading