Skip to content
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

LPS-136427 Move Some Custom Properties to Clay CSS #1277

Closed
wants to merge 5 commits into from

Conversation

pat270
Copy link

@pat270 pat270 commented Jul 22, 2021

https://issues.liferay.com/browse/LPS-136427

Hey @marcoscv-work @edalgrin @nhpatt @dsanz @matuzalemsteles @drakonux ,

I was able to move a lot of the custom properties to Clay CSS. Some places that are giving me trouble is:

$theme-colors: () !default;
$theme-colors: map-deep-merge(
	(
		primary: $primary,
		secondary: $secondary,
		success: $success,
		info: $info,
		warning: $warning,
		danger: $danger,
		light: $light,
		dark: $dark,
	),
	$theme-colors
);

$container-max-widths: (
	sm: 540px,
	md: 720px,
	lg: 960px,
	xl: 1248px,
) !default;

It's due to Bootstrap functions and mixins. I need to update those, but didn't have enough time today to figure out a work around. Anyway take a look and tell me what you think.

Edit: I've cleaned the custom properties implementation up a lot. I believe we can get rid of the custom_properties directory and bake it directly into Classic Theme. Some things that need to be updated besides the two points above:

  • When you change a color in Stylebook, it adds it to #wrapper:
<style>
  :root {
    --primary: #0b5fff;
  }
  #wrapper {
    --btn-primary-background-color: #0b5fff;
    --font-size-base: 0.875rem;
    --primary: #8e00eb;
  }
</style>

can we change this to replace the custom property inside :root? We don't need to try and scope CSS this way anymore with Cadmin.

  • All property values using a custom property should have a default value. This allows setting the Stylebook value to initial and resetting the property back to its default.
  • We probably don't need to style #wrapper anymore and just use body. I can update Clay CSS so we can customize font-size in the body tag better.
  • Why is --hr-border-color called Separation Border Color in Stylebook? It's so hard to make the connection between that label and the variable name.

Here is some markup you can toss into a fragment for testing:

<h1>h1 Article Heading <small>Sub text</small></h1>
<h2>h2 Article Heading <small>Sub text</small></h2>
<h3>h3 Article Heading <small>Sub text</small></h3>
<h4>h4 Article Heading <small>Sub text</small></h4>
<h5>h5 Article Heading <small>Sub text</small></h5>
<h6>h6 Article Heading <small>Sub text</small></h6>

<h3>Buttons</h3>
<button class="btn btn-primary" type="button">Primary</button>
<button class="btn btn-secondary" type="button">Secondary</button>
<button class="btn btn-link" type="button">Link</button>

<h3>Buttons Focus</h3>
<button class="btn btn-primary focus" type="button">Primary</button>
<button class="btn btn-secondary focus" type="button">Secondary</button>
<button class="btn btn-link focus" type="button">Link</button>

<h3>Buttons Disabled</h3>
<button class="btn btn-primary" disabled type="button">Primary</button>
<button class="btn btn-secondary" disabled type="button">Secondary</button>
<button class="btn btn-link" disabled type="button">Link</button>

<h3>Spacer 0</h3>
<div class="pt-0 bg-dark"></div>
<h3>Spacer 1</h3>
<div class="pt-1 bg-dark"></div>
<h3>Spacer 2</h3>
<div class="pt-2 bg-dark"></div>
<h3>Spacer 3</h3>
<div class="pt-3 bg-dark"></div>
<h3>Spacer 4</h3>
<div class="pt-4 bg-dark"></div>
<h3>Spacer 5</h3>
<div class="pt-5 bg-dark"></div>
<h3>Spacer 6</h3>
<div class="pt-6 bg-dark"></div>
<h3>Spacer 7</h3>
<div class="pt-7 bg-dark"></div>
<h3>Spacer 8</h3>
<div class="pt-8 bg-dark"></div>
<h3>Spacer 9</h3>
<div class="pt-9 bg-dark"></div>
<h3>Spacer 10</h3>
<div class="pt-10 bg-dark"></div>

@liferay-continuous-integration
Copy link
Collaborator

Please only forward critical changes to Brian Chan during stabilization. Nonurgent changes should wait until the ongoing 7.4 DXP EP3 & CE GA3 release has been completed. For more details on the release timeline and status, see product-delivery.

@liferay-continuous-integration
Copy link
Collaborator

CI is automatically triggering the following test suites:

  •     ci:test:relevant
  •     ci:test:sf

@pat270
Copy link
Author

pat270 commented Jul 22, 2021

ci:stop

@liferay-continuous-integration
Copy link
Collaborator

❌ ci:test:sf - 0 out of 1 jobs passed in 5 minutes

Click here for more details.

Base Branch:

Branch Name: master
Branch GIT ID: f47c6ac081ab80c4c744698e185d77ade2758188

Sender Branch:

Branch Name: custom-properties
Branch GIT ID: 7f30fb083257cbc527fb9f3e14b1848103ff8c8b

0 out of 1jobs PASSED
For more details click here.
     [java] 	at com.liferay.source.formatter.SourceFormatter.main(SourceFormatter.java:287)
[stopwatch] [run.batch.test.action: 2:45.950 sec]
     [echo] The following error occurred while executing this line:
     [echo] /opt/dev/projects/github/liferay-portal/portal-impl/build.xml:572: The following error occurred while executing this line:
     [echo] /opt/dev/projects/github/liferay-portal/portal-impl/build.xml:706: Java returned: 1
      [get] Getting: http://test-1-4/job/test-portal-source-format/5089//consoleText
      [get] To: /opt/dev/projects/github/liferay-portal/20210722160425077.txt
   [delete] Deleting: /opt/dev/projects/github/liferay-portal/20210722160425077.txt
  [typedef] Could not load definitions from resource org/apache/maven/artifact/ant/antlib.xml. It could not be found.
  [taskdef] Could not load definitions from resource org/jacoco/ant/antlib.xml. It could not be found.
   [delete] Deleting: /opt/dev/projects/github/liferay-portal/null1036310812.properties

merge-test-results:
[mkdir] Created dir: /opt/dev/projects/github/liferay-portal/test-results/src
[beanshell] Copying /opt/dev/projects/github/liferay-portal/portal-impl/test-results/TEST-JenkinsLogAssertTest.xml to /opt/dev/projects/github/liferay-portal/test-results/src/TEST-JenkinsLogAssertTest.xml
[junitreport] Processing /opt/dev/projects/github/liferay-portal/test-results/TESTS-TestSuites.xml to /tmp/null364267786
[junitreport] Loading stylesheet jar:file:/opt/java/ant/lib/ant-junit.jar!/org/apache/tools/ant/taskdefs/optional/junit/xsl/junit-frames.xsl
[junitreport] Transform time: 176ms
[junitreport] Deleting: /tmp/null364267786
[echo] A report with all the test results can be found at test-results/html/index.html.
[mkdir] Created dir: /opt/dev/projects/github/liferay-jenkins-ee/test-results
[copy] Copying 1 file to /opt/dev/projects/github/liferay-jenkins-ee/test-results
[echo] run.batch.test.tear.down.start.timestamp: 07-22-2021 16:04:28:345 PDT
[stopwatch] [run.batch.test.tear.down: 0.000 sec]
[typedef] Could not load definitions from resource org/apache/maven/artifact/ant/antlib.xml. It could not be found.
[taskdef] Could not load definitions from resource org/jacoco/ant/antlib.xml. It could not be found.
[delete] Deleting: /opt/dev/projects/github/liferay-portal/null1036310812.properties

print-gc-logs:
[echo]
[echo] ##
[echo] ## /tmp/ant-gc.log
[echo] ##
[echo]

@liferay-continuous-integration
Copy link
Collaborator

@liferay-continuous-integration
Copy link
Collaborator

✔️ ci:test:stable - 9 out of 9 jobs passed

✔️ ci:test:relevant - 22 out of 22 jobs passed in 1 hour 40 minutes

Click here for more details.

Base Branch:

Branch Name: master
Branch GIT ID: f47c6ac081ab80c4c744698e185d77ade2758188

Upstream Comparison:

Branch GIT ID: b5bd25d65e74ce19974cd5a7ff3443cb0f022b79
Jenkins Build URL: Acceptance Upstream DXP (master) #2134

ci:test:stable - 9 out of 9 jobs PASSED
9 Successful Jobs:
ci:test:relevant - 22 out of 22 jobs PASSED
22 Successful Jobs:
For more details click here.
Test bundle downloads:

@liferay-continuous-integration
Copy link
Collaborator

@javiergamarra
Copy link

@marcoscv-work , @edalgrin comment plz

@edalgrin
Copy link
Collaborator

I'm taking a look 👀

Copy link
Collaborator

@edalgrin edalgrin left a comment

Choose a reason for hiding this comment

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

The code seems really good (I'm going to try it, in the portal, later), just a couple of questions

  • Couldn't we remove the variables that we don't need for the custom properties?
  • Is the !default flag necessary?

Apart from that, I would love to be able to select just 1 color as primary and automatically obtain primary-d1, primary-d2, etc (or in this case: hover, focus, active). It's something they required for Stylebook but could get. With the current system is not possible because we are using SASS (preprocessor) to get those, ideally we should switch to HSL colors that allow calculations through custom-properties (example) but it requires a redefinitions of the colors and probably a sort of standardization: use the same amount of lighten or darken among all the colors (5.1 != 5)

--blockquote-font-size: #{$font-size-base} * 1.25;
--blockquote-small-color: #{$gray-600};
--blockquote-small-font-size: #{$small-font-size};
--blockquote-font-size: #{getDefault($blockquote-font-size)};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since we are already providing a fallback in the custom properties declarations I think we don't need to redefine all these (👇 ) custom properties, they aren't' used anywhere (that's one of the changes I wanted to to in the layer edalgrin#121)

Copy link
Author

Choose a reason for hiding this comment

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

Yes, if it's not needed we can remove them.

Comment on lines 444 to 458
$border-color: #f1f2f5;
$bright-color: #1865fb;
$complementary-color: #30313f;
$dark-color: #272833;
$input-bg: #f1f2f5;
$light-color: #e7e7ed;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I remember taking a look at these and finding out that they were useless improvable, maybe we aren't use use them anymore or we can use the clay variable instead of new ones. I wanted to review these on the upcoming epic https://issues.liferay.com/browse/LPS-134071 but since we are here..

Copy link
Collaborator

Choose a reason for hiding this comment

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

Fixed here brianchandotcom@6df096f (we just need to rebase this PR)

@marcoscv-work
Copy link

it looks very promising

@edalgrin
Copy link
Collaborator

Just started reviewing :)

Copy link
Collaborator

@edalgrin edalgrin left a comment

Choose a reason for hiding this comment

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

Testing the change in the portal it works wonderfully! 😄
test

There is a problem with the .rounded class that used $border-radius and it's defined in the Stylebook.
error-rounded

By the way, we have a widget to test these (although we don't have buttons in there 🤔)
sample-widget

Apart from the previous comments and this new one 👆, it seems almost ready! Perhaps we should add the custom properties for Dialect (padding, font, etc..) and wait/ask for what might be useful to others team/project as well? (lol, commerce, forms)

@pat270
Copy link
Author

pat270 commented Jul 23, 2021

Is the !default flag necessary?

!default and map-merge's are only necessary if you want to allow someone to overwrite Sass variables at the top of the file. You can remove them. I copy pasted the global variables from Clay CSS.

Apart from that, I would love to be able to select just 1 color as primary and automatically obtain primary-d1, primary-d2, etc (or in this case: hover, focus, active).

This doesn't translate well for all colors in real life. Just look at the differences in the lightness values of Lexicon's (https://liferay.design/lexicon/foundations/color/) color palette. Primary, Secondary, Light, and Error aren't consistently stepped. We can't automagically provide this. Stylebooks also seem like it should be a general way to customize, meaning we should support all valid CSS color values.

@pat270
Copy link
Author

pat270 commented Jul 23, 2021

@edalgrin Thanks, I wish I knew about this widget.

@matuzalemsteles
Copy link

Well, this looks very interesting and promising, I have some concerns about how much it can grow when I include variables for other components. In the end they are APIs that we will have to maintain for the long term.

@edalgrin
Copy link
Collaborator

This doesn't translate well for all colors in real life. Just look at the differences in the lightness values of Lexicon's (https://liferay.design/lexicon/foundations/color/) color palette. Primary, Secondary, Light, and Error aren't consistently stepped. We can't automagically provide this. Stylebooks also seem like it should be a general way to customize, meaning we should support all valid CSS color values.

I was talking with @emiliano-cicero and he was working on a similar concept https://www.figma.com/file/wWYQHFoBnCxTjKKnYta19I/Color-palette-for-illustrations, perhaps we could make it compatible with Lexicon?
/cc @drakonux @hold-shift

@ghost
Copy link

ghost commented Jul 27, 2021

Hello everyone, all this looks really exciting.

The illustration palettes concept was an exercise designed to work on images with a great visual richness but not intended to be applied to the product with constraints such as accessibility.

About creating a color palette for Lexicon, we could develop a way to check color pairings' contrast accessibility. At the same time, each color would be consistently stepped in each palette gradient.

Some time ago, I worked on a project where we studied a model to create palettes based on the magic number, creating color palettes that comply with contrast accessibility. We created several palettes with values ​​between 0 (white) and 100 (black) depending on their luminance value to get the contrast ratio with a simple subtract operation of a pair of colors values.

A magic number of 40+ ensures a contrast ratio of 3+
A magic number of 50+ ensures a contrast ratio of 4.5+
A magic number of 70+ ensures a contrast ratio of 7+

You can see more about the process in this video where we presented the color patterns building model.

Regarding the scalability that @matuzalemsteles talks about, I have added a high-level description of the process and some reference sources in the Lexicon Design Tokens Research document.

Among all, I believe that Amazon Style Dictionary is the standard that has achieved a scalable model that can also be applied to our tokenization model, but please tell me what you think about it.

Thanks.

@matuzalemsteles
Copy link

@hold-shift it's a good job alias, I've seen some of these libs implementing the Design Tokens idea or spreading this idea I see that the big benefit of it really is offering configurations for multiple platforms.

One of the concerns is how we balance the use of global tokens with component-specific tokens, for example the definition of the new Button component, we have some component-specific tokens that could be related to the global ones.

  • --btn-font-*

How would we be doing this balance to use global tokens instead of adding component-specific tokens.

Another point when I talk about is the granularity of specific tokens for the component, for example:

  • --btn-min-height
  • --btn-border-*
  • --btn-padding-*

Could these tokens escape the definition? probably they are the key piece to allow Dialect to create its own components, but would that be deviating from that and looking at Dialect's Figma could it repeat for other components? My concern is more with how we scale this granularity, we will probably know more when we have the analysis of the other components. I've added a few more info about it, in case you're interested liferay/clay#4174

I liked the idea of versioning and status for tokens, these are good things to keep track of like we try to do in React components, we don't have the test definition but it would be great before committing to it for the long term.

background-color: var(--btn-primary-background-color, $primary),
border-color: var(--btn-primary-border-color, $primary),
box-shadow: $c-unset,
color: var(--btn-primary-color, color-yiq($primary)),

Choose a reason for hiding this comment

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

In these cases we shouldn't use --primary token instead of creating a component-specific one, it seems a bit inconsistent, for example in theory btn-primary would follow the colors of primary, as well as subsequent ones, with would that allow you to change the color of btn-primary without following the global color of --primary, in which case wouldn't it be a bit contradictory? And if the user wants a different color for the button, wouldn't it be a new variation?

@pat270
Copy link
Author

pat270 commented Jul 27, 2021

I worked on a project where we studied a model to create palettes based on the magic number, creating color palettes that comply with contrast accessibility. We created several palettes with values ​​between 0 (white) and 100 (black) depending on their luminance value to get the contrast ratio with a simple subtract operation of a pair of colors values.

Bootstrap 5 has a function based on luminance that outputs black or white text depending on the background-color. Bootstrap 4 has something similar based on the yiq color space. It auto-generates a color for you based on a background-color you pass into a function. We can easily modify it to output a specific color based on colors you pass into it, but you have to input the color you want. The big problem here is we are assuming colors should 1 dimensional (e.g., up or down on the primary scale for buttons).

What if I want to shift left or right (e.g., my primary button to be yellow / black on active)? There's nothing in the magic number equation that will do that for me. It just tells me if two colors satisfy a contrast ratio. It doesn't pick the color I want. This is the main reason we shouldn't implement this. It adds tons of complexity for no real benefit.

@javiergamarra
Copy link

ci:test:relevant

@liferay-continuous-integration
Copy link
Collaborator

✔️ ci:test:stable - 9 out of 9 jobs passed

✔️ ci:test:relevant - 22 out of 22 jobs passed in 1 hour 24 minutes

Click here for more details.

Base Branch:

Branch Name: master
Branch GIT ID: 7cb912889719a27e3dff73a0501155c0f36610f9

Upstream Comparison:

Branch GIT ID: 7cb912889719a27e3dff73a0501155c0f36610f9
Jenkins Build URL: Acceptance Upstream DXP (master) #2168

ci:test:stable - 9 out of 9 jobs PASSED
9 Successful Jobs:
ci:test:relevant - 22 out of 22 jobs PASSED
22 Successful Jobs:
For more details click here.
Test bundle downloads:

@liferay-continuous-integration
Copy link
Collaborator

@javiergamarra
Copy link

let's break everything then :D

@javiergamarra
Copy link

ci:forward

@liferay-continuous-integration
Copy link
Collaborator

CI is automatically triggering the following test suites:

  •     ci:test:relevant
  •     ci:test:sf

The pull request will automatically be forwarded to the user brianchandotcom if the following test suites pass:

  •     ci:test:relevant
  •     ci:test:sf
  •     ci:test:stable

@liferay-continuous-integration
Copy link
Collaborator

Skipping previously passed test suites:
ci:test:relevant

@liferay-continuous-integration
Copy link
Collaborator

✔️ ci:test:sf - 1 out of 1 jobs passed in 2 minutes

Click here for more details.

Base Branch:

Branch Name: master
Branch GIT ID: 7cb912889719a27e3dff73a0501155c0f36610f9

Sender Branch:

Branch Name: custom-properties
Branch GIT ID: d363d9a9b65885a7b071e9f47312b7935c7050c6

1 out of 1jobs PASSED
1 Successful Jobs:
For more details click here.

@liferay-continuous-integration
Copy link
Collaborator

All required test suite(s) passed.
Forwarding pull request to brianchandotcom.
Console

@liferay-continuous-integration
Copy link
Collaborator

Pull request has been successfully forwarded to brianchandotcom#104983
Console

@liferay-continuous-integration
Copy link
Collaborator

@edalgrin
Copy link
Collaborator

edalgrin commented Aug 2, 2021

Not sure we wanted to add custom properties outside the custom properties folder 😬 https://github.com/liferay-frontend/liferay-portal/pull/1277/files#diff-07100ecd816943034e37b0128d4cc5364467959e487dec90db72c6eea3164106L1-R25

@pat270
Copy link
Author

pat270 commented Aug 3, 2021

@edalgrin what's the reason for we wanted to add custom properties outside the custom properties folder?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants