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.

Sign up for GitHub

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

Remove smart border style setting behavior #49714

Closed
wants to merge 4 commits into from
Closed

Remove smart border style setting behavior #49714

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
de34a9b
Remove smart border style setting behavior
youknowriad Apr 11, 2023
0aae364
Add padding style fallback
youknowriad Apr 13, 2023
c384548
Fix phpcs
youknowriad Apr 13, 2023
b3dc4b2
Update block supports
youknowriad Apr 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 20 additions & 9 deletions lib/block-supports/border.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,15 +63,6 @@ function gutenberg_apply_border_support( $block_type, $block_attributes ) {
$border_block_styles['radius'] = $border_radius;
}

// Border style.
if (
gutenberg_has_border_feature_support( $block_type, 'style' ) &&
isset( $block_attributes['style']['border']['style'] ) &&
! wp_should_skip_block_supports_serialization( $block_type, '__experimentalBorder', 'style' )
) {
$border_block_styles['style'] = $block_attributes['style']['border']['style'];
}

// Border width.
if (
$has_border_width_support &&
Expand All @@ -98,6 +89,20 @@ function gutenberg_apply_border_support( $block_type, $block_attributes ) {
$border_block_styles['color'] = $preset_border_color ? $preset_border_color : $custom_border_color;
}

// Border style.
if (
gutenberg_has_border_feature_support( $block_type, 'style' ) &&
isset( $block_attributes['style']['border']['style'] ) &&
! wp_should_skip_block_supports_serialization( $block_type, '__experimentalBorder', 'style' )
) {
$border_block_styles['style'] = $block_attributes['style']['border']['style'];
}

// If we set a border color or a border width, make sure we set a border style as well.
if ( isset( $border_block_styles['color'] ) || isset( $border_block_styles['width'] ) ) {
$border_block_styles['style'] = isset( $border_block_styles['style'] ) ? $border_block_styles['style'] : 'solid';
}

// Generate styles for individual border sides.
if ( $has_border_color_support || $has_border_width_support ) {
foreach ( array( 'top', 'right', 'bottom', 'left' ) as $side ) {
Expand All @@ -107,6 +112,12 @@ function gutenberg_apply_border_support( $block_type, $block_attributes ) {
'color' => isset( $border['color'] ) && ! wp_should_skip_block_supports_serialization( $block_type, '__experimentalBorder', 'color' ) ? $border['color'] : null,
'style' => isset( $border['style'] ) && ! wp_should_skip_block_supports_serialization( $block_type, '__experimentalBorder', 'style' ) ? $border['style'] : null,
);

// If we set a border color or a border width, make sure we set a border style as well.
if ( isset( $border_side_values['color'] ) || isset( $border_side_values['width'] ) ) {
$border_side_values['style'] = isset( $border_side_values['style'] ) ? $border_side_values['style'] : 'solid';
}

$border_block_styles[ $side ] = $border_side_values;
}
}
Expand Down
15 changes: 15 additions & 0 deletions lib/class-wp-theme-json-gutenberg.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1848,6 +1848,21 @@ protected static function compute_style_properties( $styles, $settings = array()

$root_variable_duplicates = array();

// Add a solid style fallback if no style defined.
if ( isset( $styles['border'] ) ) {
if (
( isset( $styles['border']['width'] ) || isset( $styles['border']['color'] ) ) && ! isset( $styles['border']['style'] ) ) {
$styles['border']['style'] = 'solid';
}
foreach ( array( 'top', 'left', 'right', 'bottom' ) as $edge ) {
if (
isset( $styles['border'][ $edge ]) &&
( isset( $styles['border'][ $edge ]['width'] ) || isset( $styles['border'][ $edge ]['color'] ) ) && ! isset( $styles['border'][ $edge ]['style'] ) ) {
$styles['border'][ $edge ]['style'] = 'solid';
}
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this "declarative approach" of one style object property generates one CSS rule is showing its limit in the style generation. There's so much edge cases in this file and it keeps growing. The other point here is that block supports do the exact same thing, so the code for style generation is duplicated.

The frontend side is a bit better and I think this highlights that we should move to a similar approach in the backend. (Style generation engine applied for both block supports and theme.json style generation). Not sure how that work is going.


foreach ( $properties as $css_property => $value_path ) {
$value = static::get_property_value( $styles, $value_path, $theme_json );

Expand Down
31 changes: 1 addition & 30 deletions packages/block-editor/src/components/global-styles/border-panel.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,35 +45,6 @@ function useHasBorderWidthControl( settings ) {
return settings?.border?.width;
}

function applyFallbackStyle( border ) {
if ( ! border ) {
return border;
}

if ( ! border.style && ( border.color || border.width ) ) {
return { ...border, style: 'solid' };
}

return border;
}

function applyAllFallbackStyles( border ) {
if ( ! border ) {
return border;
}

if ( hasSplitBorders( border ) ) {
return {
top: applyFallbackStyle( border.top ),
right: applyFallbackStyle( border.right ),
bottom: applyFallbackStyle( border.bottom ),
left: applyFallbackStyle( border.left ),
};
}

return applyFallbackStyle( border );
}

function BorderToolsPanel( {
resetAllFilter,
onChange,
Expand Down Expand Up @@ -186,7 +157,7 @@ export default function BorderPanel( {
const onBorderChange = ( newBorder ) => {
// Ensure we have a visible border style when a border width or
// color is being selected.
const updatedBorder = applyAllFallbackStyles( newBorder );
const updatedBorder = newBorder ? { ...newBorder } : undefined;

if ( hasSplitBorders( updatedBorder ) ) {
[ 'top', 'right', 'bottom', 'left' ].forEach( ( side ) => {
Expand Down
43 changes: 0 additions & 43 deletions packages/block-library/src/common.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,49 +104,6 @@
z-index: 100000;
}

/**
* The following provide a simple means of applying a default border style when
* a user first makes a selection in the border block support panel.
* This prevents issues such as where the user could set a border width
* and see no border due there being no border style set.
*
* This is intended to be removed once intelligent defaults can be set while
* making border selections via the block support.
*
* See: https://github.com/WordPress/gutenberg/pull/33743
*/
html :where(.has-border-color) {
border-style: solid;
}
html :where([style*="border-top-color"]) {
border-top-style: solid;
}
html :where([style*="border-right-color"]) {
border-right-style: solid;
}
html :where([style*="border-bottom-color"]) {
border-bottom-style: solid;
}
html :where([style*="border-left-color"]) {
border-left-style: solid;
}

html :where([style*="border-width"]) {
border-style: solid;
}
html :where([style*="border-top-width"]) {
border-top-style: solid;
}
html :where([style*="border-right-width"]) {
border-right-style: solid;
}
html :where([style*="border-bottom-width"]) {
border-bottom-style: solid;
}
html :where([style*="border-left-width"]) {
border-left-style: solid;
}

/**
* Provide baseline responsiveness for images.
*/
Expand Down
80 changes: 76 additions & 4 deletions packages/style-engine/src/styles/border/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,13 @@
/**
* Internal dependencies
*/
import type { BoxEdge, GenerateFunction, StyleDefinition } from '../../types';
import type {
BoxEdge,
GenerateFunction,
StyleDefinition,
Style,
StyleOptions,
} from '../../types';
import { generateRule, generateBoxRules, camelCaseJoin } from '../utils';

/**
Expand All @@ -26,15 +32,56 @@ function createBorderGenerateFunction( path: string[] ): GenerateFunction {
function createBorderEdgeGenerateFunction( edge: BoxEdge ): GenerateFunction {
return ( style, options ) => {
return [ 'color', 'style', 'width' ].flatMap( ( key ) => {
let styleWithFallback = style;
if (
( style?.border?.[ edge ]?.color ||
style?.border?.[ edge ]?.width ) &&
! style?.border?.[ edge ]?.style
) {
styleWithFallback = {
...style,
border: {
...style?.border,
[ edge ]: {
...style?.border?.[ edge ],
style: 'solid',
},
},
};
}
const path = [ 'border', edge, key ];
return createBorderGenerateFunction( path )( style, options );
return createBorderGenerateFunction( path )(
styleWithFallback,
options
);
} );
};
}

const color: StyleDefinition = {
name: 'color',
generate: createBorderGenerateFunction( [ 'border', 'color' ] ),
generate: ( style: Style, options: StyleOptions ) => {
const borderColorGenerate = createBorderGenerateFunction( [
'border',
'color',
] );
const borderStyleGenerate = createBorderGenerateFunction( [
'border',
'style',
] );
const output = borderColorGenerate( style, options );
if ( style?.border?.color && ! style?.border?.style ) {
return [
...output,
...borderStyleGenerate(
{ border: { style: 'solid' } },
options
),
];
}

return output;
},
};

const radius: StyleDefinition = {
Expand All @@ -60,7 +107,32 @@ const borderStyle: StyleDefinition = {

const width: StyleDefinition = {
name: 'width',
generate: createBorderGenerateFunction( [ 'border', 'width' ] ),
generate: ( style: Style, options: StyleOptions ) => {
const borderWidthGenerate = createBorderGenerateFunction( [
'border',
'width',
] );
const borderStyleGenerate = createBorderGenerateFunction( [
'border',
'style',
] );
const output = borderWidthGenerate( style, options );
if (
style?.border?.width &&
! style?.border.color && // this is only here to avoid duplication
! style?.border?.style
) {
return [
...output,
...borderStyleGenerate(
{ border: { style: 'solid' } },
options
),
];
}

return output;
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

One interesting thing to note here is that the architecture of these rules is "too abstracted", Basically, I have to repeat the same fallback code because it's organized as if "one style property" = "one CSS rule output" and it shouldn't be because "style" is an abstraction and not just another way to write CSS.

Anyway, I managed to make it work and the good thing is that for the frontend at least, I had to update only the style engine to make the fallback work for both global styles and block supports.

};

const borderTop: StyleDefinition = {
Expand Down
36 changes: 35 additions & 1 deletion packages/style-engine/src/test/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,40 @@ describe( 'generate', () => {
);
} );

it( 'should geneate a border style fallback if color provided', () => {
expect(
compileCSS( {
border: {
color: 'var:preset|color|perky-peppermint',
},
} )
).toEqual(
'border-color: var(--wp--preset--color--perky-peppermint); border-style: solid;'
);
} );

it( 'should geneate a border style fallback if width provided', () => {
expect(
compileCSS( {
border: {
width: '5px',
},
} )
).toEqual( 'border-width: 5px; border-style: solid;' );
} );

it( 'should geneate a border style fallback for individual border rules too', () => {
expect(
compileCSS( {
border: {
top: {
width: '5px',
},
},
} )
).toEqual( 'border-top-style: solid; border-top-width: 5px;' );
} );

it( 'should parse individual border rules', () => {
expect(
compileCSS( {
Expand Down Expand Up @@ -166,7 +200,7 @@ describe( 'generate', () => {
},
} )
).toEqual(
'border-top-left-radius: 1px; border-top-right-radius: 2px; border-bottom-left-radius: 3px; border-bottom-right-radius: 4px; border-top-color: var(--wp--preset--color--sandy-beach); border-top-style: dashed; border-top-width: 9px; border-right-color: var(--wp--preset--color--leafy-avenue); border-right-width: 5rem; border-bottom-color: #eee; border-bottom-style: solid; border-bottom-width: 2%; border-left-color: var(--wp--preset--color--avocado-blues); border-left-style: dotted; border-left-width: 100px;'
'border-top-left-radius: 1px; border-top-right-radius: 2px; border-bottom-left-radius: 3px; border-bottom-right-radius: 4px; border-top-color: var(--wp--preset--color--sandy-beach); border-top-style: dashed; border-top-width: 9px; border-right-color: var(--wp--preset--color--leafy-avenue); border-right-style: solid; border-right-width: 5rem; border-bottom-color: #eee; border-bottom-style: solid; border-bottom-width: 2%; border-left-color: var(--wp--preset--color--avocado-blues); border-left-style: dotted; border-left-width: 100px;'
);
} );
} );
Expand Down