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

[AMP Stories] Add overlay settings to Page #2046

Merged
merged 6 commits into from
Apr 3, 2019
Merged

[AMP Stories] Add overlay settings to Page #2046

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
2a74e68
Add overlay to Page.
miina Apr 2, 2019
005cede
Adjust the logic for gradient.
miina Apr 2, 2019
b3bd744
Merge remote-tracking branch 'origin/amp-stories-redux' into amp-stor…
miina Apr 2, 2019
1e78f88
Tidying up and bug fixes
swissspidy Apr 3, 2019
d31323f
ESLint fix
swissspidy Apr 3, 2019
9049a7a
Remove unused attribute
swissspidy Apr 3, 2019
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
46 changes: 39 additions & 7 deletions assets/src/blocks/amp-story/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,11 +95,13 @@ class EditPage extends Component {
const { attributes, media, setAttributes, totalAnimationDuration } = this.props;

const {
backgroundColor,
gradientBottomColor,
mediaId,
mediaType,
mediaUrl,
focalPoint,
overlayColor,
overlayOpacity,
poster,
autoAdvanceAfter,
autoAdvanceAfterDuration,
Expand All @@ -108,7 +110,6 @@ class EditPage extends Component {
const instructions = <p>{ __( 'To edit the background image or video, you need permission to upload media.', 'amp' ) }</p>;

const style = {
backgroundColor,
backgroundImage: IMAGE_BACKGROUND_TYPE === mediaType && mediaUrl ? `url(${ mediaUrl })` : undefined,
backgroundPosition: IMAGE_BACKGROUND_TYPE === mediaType && focalPoint ? `${ focalPoint.x * 100 }% ${ focalPoint.y * 100 }%` : 'cover',
backgroundRepeat: 'no-repeat',
Expand All @@ -134,20 +135,48 @@ class EditPage extends Component {
autoAdvanceAfterHelp = __( 'Based on the duration of all animated blocks on the page', 'amp' );
}

const overlayStyle = {
width: '100%',
height: '100%',
};
if ( ! gradientBottomColor && overlayColor ) {
overlayStyle.backgroundColor = overlayColor;
overlayStyle.opacity = overlayOpacity / 100;
} else if ( gradientBottomColor ) {
const topColor = overlayColor ? overlayColor : 'transparent';
overlayStyle.backgroundImage = `linear-gradient(to bottom, ${ topColor }, ${ gradientBottomColor })`;
overlayStyle.opacity = overlayOpacity / 100;
}

return (
<Fragment>
<InspectorControls key="controls">
<PanelColorSettings
title={ __( 'Color Settings', 'amp' ) }
title={ __( 'Overlay Settings', 'amp' ) }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
title={ __( 'Overlay Settings', 'amp' ) }
title={ __( 'Background Color', 'amp' ) }

If one just wants to use a solid background color without any gradient, I think this wording makes more sense.

Thanks to the opacity control further down, one will quickly discover that this can be used as an overlay for the background image.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I was thinking that Overlay is a word that is valid for both cases since Overlay doesn't necessarily have to be an overlay for an image -- it can be overlay for the Page block without the image, too. Also, technically it'll be a new Fill layer (aka overlay) and not use the background color.

Since the user will not necessarily open the Panel to see what's inside then maybe Background Color could be deceiving, it would be great if the title would be self-explanatory without having to know what's inside. Maybe there's another option instead?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Background Settings maybe? Then it could be combined with the background media panel.

Technically it's a fill layer, but for the user it's basically a background color setting with some optional opacity.

For me, "background" is easier to grasp than "overlay", and also easier to localize.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Background Settings makes sense 👍

Copy link
Member

Choose a reason for hiding this comment

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

Since from the user perspective the net effect of this setting is providing a background color/fill, I would consider Background Color over `Settings. We want to make the language and UI as straightforward and intuitive as possible.

initialOpen={ false }
colorSettings={ [
{
value: backgroundColor,
onChange: ( value ) => setAttributes( { backgroundColor: value } ),
label: __( 'Background Color', 'amp' ),
value: overlayColor,
onChange: ( value ) => setAttributes( { overlayColor: value } ),
label: __( 'Color', 'amp' ),
},
{
value: gradientBottomColor,
onChange: ( value ) => setAttributes( { gradientBottomColor: value } ),
label: __( 'Bottom Color: Use for Gradient Only', 'amp' ),
},
] }
/>
>
<RangeControl
label={ __( 'Opacity', 'amp' ) }
value={ overlayOpacity }
onChange={ ( value ) => setAttributes( { overlayOpacity: value } ) }
min={ 0 }
max={ 100 }
step={ 5 }
required
/>
</PanelColorSettings>
<PanelBody title={ __( 'Background Media', 'amp' ) }>
<Fragment>
<BaseControl>
Expand Down Expand Up @@ -250,6 +279,9 @@ class EditPage extends Component {
</video>
</div>
) }
{ ( overlayColor || gradientBottomColor ) && (
<div style={ overlayStyle }></div>
) }
<InnerBlocks template={ TEMPLATE } allowedBlocks={ ALLOWED_CHILD_BLOCKS } />
</div>
</Fragment>
Expand Down
31 changes: 26 additions & 5 deletions assets/src/blocks/amp-story/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,6 @@ const schema = {
selector: 'amp-story-page',
attribute: 'id',
},
backgroundColor: {
default: '#ffffff',
},
mediaId: {
type: 'number',
},
Expand Down Expand Up @@ -49,6 +46,16 @@ const schema = {
autoAdvanceAfterMedia: {
type: 'string',
},
overlayColor: {
default: null,
},
overlayOpacity: {
default: 50,
},
gradientBottomColor: {
default: null,
type: 'string',
},
};

export const settings = {
Expand Down Expand Up @@ -78,7 +85,9 @@ export const settings = {
save( { attributes } ) {
const {
anchor,
backgroundColor,
overlayColor,
overlayOpacity,
gradientBottomColor,
mediaUrl,
mediaType,
poster,
Expand All @@ -95,8 +104,18 @@ export const settings = {
advanceAfter = autoAdvanceAfterMedia;
}

const overlayStyle = {
opacity: overlayOpacity / 100,
};
if ( ! gradientBottomColor && overlayColor ) {
overlayStyle.backgroundColor = overlayColor;
} else if ( gradientBottomColor ) {
const topColor = overlayColor ? overlayColor : 'transparent';
overlayStyle.backgroundImage = `linear-gradient(to bottom, ${ topColor }, ${ gradientBottomColor })`;
}

return (
<amp-story-page style={ { backgroundColor } } id={ anchor } auto-advance-after={ advanceAfter }>
<amp-story-page style={ { backgroundColor: '#ffffff' } } id={ anchor } auto-advance-after={ advanceAfter }>
{
mediaUrl && (
<amp-story-grid-layer template="fill">
Expand All @@ -109,6 +128,8 @@ export const settings = {
</amp-story-grid-layer>
)
}
<amp-story-grid-layer template="fill" style={ overlayStyle }>
</amp-story-grid-layer>
<amp-story-grid-layer template="vertical">
<InnerBlocks.Content />
</amp-story-grid-layer>
Expand Down