Style engine: add border to frontend #41803
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ramonjd
added
[Type] Experimental
|
Size Change: +829 B (0%) Total Size: 1.24 MB
ℹ️ View Unchanged
|
ramonjd
commented
Jun 20, 2022
| * @param options Options object with settings to adjust how the styles are generated. | ||
| * @param path An array of strings representing the path to the style value in the style object. | ||
| * @param ruleKeys An array of CSS property keys and patterns. | ||
| * @param individualProperties The "sides" or individual properties for which to generate rules. | ||
| * | ||
| * @return GeneratedCSSRule[] CSS rules. | ||
| */ | ||
| export function generateBoxRules( |
There was a problem hiding this comment.
As a follow up, maybe this could assume the responsibility of generateRule, similar to what we do in the backend.
| @@ -11,7 +11,31 @@ export type Box< T extends BoxVariants = undefined > = { | |||
| left?: CSSProperties[ T extends undefined ? 'left' : `${ T }Left` ]; | |||
| }; | |||
|
|
|||
| export type BorderIndividualProperty = 'top' | 'right' | 'bottom' | 'left'; | |||
| export type BorderIndividualStyles< T extends BorderIndividualProperty > = { | |||
| color?: CSSProperties[ `border${ Capitalize< string & T > }Color` ]; | |||
There was a problem hiding this comment.
My TS fu isn't great. Very glad to hear if there's a fancier way of doing this.
There was a problem hiding this comment.
My TS fu isn't very good, either, but this looks reasonable to me. I often find these rules a bit challenging to parse visually, but it given the sheer number of rules for each side, this reads better than the duplication of hard-coding each CSS property to me 👍
There was a problem hiding this comment.
Perhaps a simple inline comment with an example of the results might help people grok this quicker? Also, renaming T to something more explanatory could be another option.
| const borderLeft = { | ||
| name: 'borderLeft', | ||
| generate: createBorderGenerateFunction( 'left' ), | ||
| }; |
There was a problem hiding this comment.
I'm starting to think that we could refactor the JS style engine to use properties from a master "definitions" constant, which is basically what we do in hooks/style.js anyway using __EXPERIMENTAL_STYLE_PROPERTY
It's how we approached it in the backend.
And would allow us to store custom paths and CSS rules keys such as border-$s-color without the need to build them on the fly.
Not sure yet. I'll test it later.
andrewserong
approved these changes
Jun 20, 2022
There was a problem hiding this comment.
Nice one again @ramonjd! This LGTM and each of the individual border properties are testing well for me in the editor, with the same markup as prior to this PR 👍
Good question about whether to bring a single canonical list of the properties into the style engine package — it could be useful to centralise that knowledge, but it's still challenging for us to attempt to reduce the duplication between JS and PHP (I'm not sure if there's all that much we can do there for now 🤔). I suppose with the JS implementation, because things are split out into a separate style properties, we've got a bit more flexibility to have each module responsible for itself, whereas in PHP, we're a bit more bound by trying to keep it all into the one class, where a single list is very useful. At any rate, I agree it's something we could explore in follow-ups.
This change looks good to me! 🎉
| /** | ||
| * External dependencies | ||
| */ | ||
| import { camelCase, get } from 'lodash'; |
There was a problem hiding this comment.
There's some active work underway to remove lodash from the repo (#17025) — I'm not sure we can easily remove camelCase from the repo just yet, but there might be a simpler alternative for get (I've added a separate comment)
There was a problem hiding this comment.
I'm not sure we can easily remove camelCase from the repo just yet
Maybe we could do it for the style engine though. We're also using _.upperFirst(). We could kill 2 birds with one method:
const upperFirst = ( str ) => return ${ str.charAt(0).toUpperCase() + str.slice(1) };
const ruleKey = `border${ upperFirst( individualProperty ) }${ upperFirst( key ) }`;I'll replace!
There was a problem hiding this comment.
Oh, I like it! And it makes sense for the style engine to have its own little utility methods like this 🙂
| @@ -11,7 +11,31 @@ export type Box< T extends BoxVariants = undefined > = { | |||
| left?: CSSProperties[ T extends undefined ? 'left' : `${ T }Left` ]; | |||
| }; | |||
|
|
|||
| export type BorderIndividualProperty = 'top' | 'right' | 'bottom' | 'left'; | |||
| export type BorderIndividualStyles< T extends BorderIndividualProperty > = { | |||
| color?: CSSProperties[ `border${ Capitalize< string & T > }Color` ]; | |||
There was a problem hiding this comment.
My TS fu isn't very good, either, but this looks reasonable to me. I often find these rules a bit challenging to parse visually, but it given the sheer number of rules for each side, this reads better than the duplication of hard-coding each CSS property to me 👍
ramonjd
force-pushed
the
add/style-engine-border-frontend
branch
2 times, most recently
from
f4a50ce
to
409a76b
Compare
25 tasks
Adding border style types and generator functions Refactoring generateBoxRules to accommodate various "individual" properties
Removing lodash camelCase and upperFirst in favour of own implementation + test Removing unnecessary lodash get()
ramonjd
force-pushed
the
add/style-engine-border-frontend
branch
from
409a76b
to
93abab8
Compare
mburridge
added
the
Needs Dev Note
|
Added the Needs Dev Note label in case this needs a dev note (either individual or as part of a "misc" dev note) for WP 6.1 release. |
ramonjd
removed
the
Needs Dev Note
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Adding border style generation to the style engine JS package.
Related backed PR: #40531
Why?
Carrying on with consolidating styles outlined in the tracking issue:
How?
Testing Instructions
I'm using Empty Theme to test.
Create a new post and add some blocks, such as Group or Column.
Update the border styles, and play around with individual border styles.
Here is some example block code:
Example HTML
Save the post.
Check that the editor and frontend match.
Run the tests!