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

Jump to bottom

Cover Image: Add range slider for background dim #2815

Merged
merged 3 commits into from
Oct 2, 2017
Merged

Cover Image: Add range slider for background dim #2815

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f6ae7fe
Cover Image: Replace hasBackgroundDim toggle with slider control
mcsf Sep 27, 2017
eab53ef
Cover Image: Ensure backcompat via default attributes
mcsf Sep 29, 2017
7280973
Cover Image: Classes should be fraction of 100
mcsf Sep 29, 2017
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
54 changes: 36 additions & 18 deletions blocks/library/cover-image/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import BlockControls from '../../block-controls';
import BlockAlignmentToolbar from '../../block-alignment-toolbar';
import InspectorControls from '../../inspector-controls';
import ToggleControl from '../../inspector-controls/toggle-control';
import RangeControl from '../../inspector-controls/range-control';
import BlockDescription from '../../block-description';

const { children } = source;
Expand Down Expand Up @@ -48,9 +49,9 @@ registerBlockType( 'core/cover-image', {
type: 'boolean',
default: false,
},
hasBackgroundDim: {
type: 'boolean',
default: true,
dimRatio: {
type: 'number',
default: 50,
},
},

Expand All @@ -62,18 +63,22 @@ registerBlockType( 'core/cover-image', {
},

edit( { attributes, setAttributes, focus, setFocus, className } ) {
const { url, title, align, id, hasParallax, hasBackgroundDim } = attributes;
const { url, title, align, id, hasParallax, dimRatio } = attributes;
const updateAlignment = ( nextAlign ) => setAttributes( { align: nextAlign } );
const onSelectImage = ( media ) => setAttributes( { url: media.url, id: media.id } );
const toggleParallax = () => setAttributes( { hasParallax: ! hasParallax } );
const toggleBackgroundDim = () => setAttributes( { hasBackgroundDim: ! hasBackgroundDim } );
const setDimRatio = ( ratio ) => setAttributes( { dimRatio: ratio } );
const style = url
? { backgroundImage: `url(${ url })` }
: undefined;
const classes = classnames( className, {
'has-parallax': hasParallax,
'has-background-dim': hasBackgroundDim,
} );
const classes = classnames(
className,
dimRatioToClass( dimRatio ),
{
'has-background-dim': dimRatio !== 0,
'has-parallax': hasParallax,
}
);

const controls = focus && [
<BlockControls key="controls">
Expand Down Expand Up @@ -108,10 +113,13 @@ registerBlockType( 'core/cover-image', {
checked={ !! hasParallax }
onChange={ toggleParallax }
/>
<ToggleControl
label={ __( 'Dim Background' ) }
checked={ !! hasBackgroundDim }
onChange={ toggleBackgroundDim }
<RangeControl
label={ __( 'Background Dimness' ) }
value={ dimRatio }
onChange={ setDimRatio }
min={ 0 }
max={ 100 }
step={ 10 }
/>
</InspectorControls>,
];
Expand Down Expand Up @@ -161,14 +169,18 @@ registerBlockType( 'core/cover-image', {
},

save( { attributes, className } ) {
const { url, title, hasParallax, hasBackgroundDim } = attributes;
const { url, title, hasParallax, dimRatio } = attributes;
const style = url
? { backgroundImage: `url(${ url })` }
: undefined;
const classes = classnames( className, {
'has-parallax': hasParallax,
'has-background-dim': hasBackgroundDim,
} );
const classes = classnames(
className,
dimRatioToClass( dimRatio ),
{
'has-background-dim': dimRatio !== 0,
Copy link
Member

Choose a reason for hiding this comment

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

Was this added to try to preserve backwards compatibility with existing content? I'd have hoped it would be the case, but unfortunately existing cover image blocks appear to be flagged as invalid. The issue appears to be that the old content will have the has-background-dim class, but the dimRatio will not be found and therefore default to 0, thus causing a subsequent save to save without has-background-dim and cause content loss (the validator in action!). Maybe we should assign the default for dimRatio to mimic what had previously existed as the default for hasBackgroundDim effect?

(For debugging validation, see also new #2837)

Copy link
Contributor Author

@mcsf mcsf Sep 29, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Was this added to try to preserve backwards compatibility with existing content?

Not really. For the issue of compatibility, I stopped to consider what we could do at the framework level. Regardless, something as small as this shouldn't be blocked by that, so I pushed 2884db eab53ef. Note, though, the weirdness of

 function dimRatioToClass( ratio ) {
-       return ratio === 0
+       return ( ratio === 0 || ratio === 50 )
                ? null
                : 'has-background-dim-' + Math.round( ratio / 10 );
 }

Since 50% dimming corresponds to the previous default value of hasBackgroundDim, we need to treat that specifically to avoid altering our block's class list.

Copy link
Member

@aduth aduth Sep 29, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Not really.

If not, why do we need has-background-dim at all? (Edit: I say this, though it may allow us to keep compatibility)

Copy link
Member

Choose a reason for hiding this comment

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

Note, though, the weirdness of

I think this is fine, expected even. Similar to how we omit default values from serialized comments, we can omit the default value (50) from the class set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So... my answer was for the past only. :)

I say this, though it may allow us to keep compatibility

Exactly. At first, it lingered while I pondered on the general compat stuff, but with that eab53ef it makes sense to keep it.

'has-parallax': hasParallax,
}
);

return (
<section className={ classes } style={ style }>
Expand All @@ -177,3 +189,9 @@ registerBlockType( 'core/cover-image', {
);
},
} );

function dimRatioToClass( ratio ) {
return ( ratio === 0 || ratio === 50 )
? null
: 'has-background-dim-' + ( 10 * Math.round( ratio / 10 ) );
}
8 changes: 7 additions & 1 deletion blocks/library/cover-image/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,13 @@
left: 0;
bottom: 0;
right: 0;
background: rgba( 0,0,0,.5 );
background: rgba( black, 0.5 );
}

@for $i from 1 through 10 {
&.has-background-dim.has-background-dim-#{ $i * 10 }:before {
background-color: rgba( black, $i * 0.1 );
}
}

&.components-placeholder {
Expand Down
4 changes: 2 additions & 2 deletions blocks/test/fixtures/core__cover-image.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
<!-- wp:core/cover-image {"url":"https://cldup.com/uuUqE_dXzy.jpg","hasBackgroundDim":true} -->
<section class="wp-block-cover-image has-background-dim" style="background-image:url(https://cldup.com/uuUqE_dXzy.jpg);">
<!-- wp:core/cover-image {"url":"https://cldup.com/uuUqE_dXzy.jpg","dimRatio":40} -->
<section class="wp-block-cover-image has-background-dim-40 has-background-dim" style="background-image:url(https://cldup.com/uuUqE_dXzy.jpg);">
<h2>Guten Berg!</h2>
</section>
<!-- /wp:core/cover-image -->
4 changes: 2 additions & 2 deletions blocks/test/fixtures/core__cover-image.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,8 @@
],
"url": "https://cldup.com/uuUqE_dXzy.jpg",
"hasParallax": false,
"hasBackgroundDim": true
"dimRatio": 40
},
"originalContent": "<section class=\"wp-block-cover-image has-background-dim\" style=\"background-image:url(https://cldup.com/uuUqE_dXzy.jpg);\">\n <h2>Guten Berg!</h2>\n</section>"
"originalContent": "<section class=\"wp-block-cover-image has-background-dim-40 has-background-dim\" style=\"background-image:url(https://cldup.com/uuUqE_dXzy.jpg);\">\n <h2>Guten Berg!</h2>\n</section>"
}
]
4 changes: 2 additions & 2 deletions blocks/test/fixtures/core__cover-image.parsed.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,9 @@
"blockName": "core/cover-image",
"attrs": {
"url": "https://cldup.com/uuUqE_dXzy.jpg",
"hasBackgroundDim": true
"dimRatio": 40
},
"rawContent": "\n<section class=\"wp-block-cover-image has-background-dim\" style=\"background-image:url(https://cldup.com/uuUqE_dXzy.jpg);\">\n <h2>Guten Berg!</h2>\n</section>\n"
"rawContent": "\n<section class=\"wp-block-cover-image has-background-dim-40 has-background-dim\" style=\"background-image:url(https://cldup.com/uuUqE_dXzy.jpg);\">\n <h2>Guten Berg!</h2>\n</section>\n"
},
{
"attrs": {},
Expand Down
4 changes: 2 additions & 2 deletions blocks/test/fixtures/core__cover-image.serialized.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
<!-- wp:core/cover-image {"url":"https://cldup.com/uuUqE_dXzy.jpg"} -->
<section class="wp-block-cover-image has-background-dim" style="background-image:url(https://cldup.com/uuUqE_dXzy.jpg);">
<!-- wp:core/cover-image {"url":"https://cldup.com/uuUqE_dXzy.jpg","dimRatio":40} -->
<section class="wp-block-cover-image has-background-dim-40 has-background-dim" style="background-image:url(https://cldup.com/uuUqE_dXzy.jpg);">
<h2>Guten Berg!</h2>
</section>
<!-- /wp:core/cover-image -->