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

[amp-stories sub-task] CTA UI improvements #1636

Merged
merged 16 commits into from
Nov 22, 2018
Merged

[amp-stories sub-task] CTA UI improvements #1636

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
4fa9923
1562: Block insertion of cta layer in first amp story page
jacobschweitzer Nov 21, 2018
f966c72
Merge branch 'amp-stories' into amp-story/1562
jacobschweitzer Nov 21, 2018
26776a7
Fix blurred element issue when selecting AMP CTA child
jacobschweitzer Nov 21, 2018
3e90bd1
Do not try to add class if no element exists
jacobschweitzer Nov 21, 2018
3e1aac7
Add RichText template with placeholder
jacobschweitzer Nov 21, 2018
86c8209
Remove template as it gives error on new CTA creation
jacobschweitzer Nov 21, 2018
6815d2a
1562: Use core/button as template
jacobschweitzer Nov 22, 2018
befd00e
1562: Use else if to prevent error when trying to remove block twice
jacobschweitzer Nov 22, 2018
5211097
Use data-amp-selected attribute to identify parent CTA layer
jacobschweitzer Nov 22, 2018
96aa5c6
1562: Update props instead of using block selector for AMP story cta …
jacobschweitzer Nov 22, 2018
16bf325
Merge remote-tracking branch 'origin/amp-stories' into amp-story/1562
miina Nov 22, 2018
453441d
Add missing text domain.
miina Nov 22, 2018
703c0e5
1562: Only add data-amp-selected to CTA layer
jacobschweitzer Nov 22, 2018
b37d430
Merge branch 'amp-story/1562' of https://github.com/Automattic/amp-wp…
jacobschweitzer Nov 22, 2018
dae0bcb
Improve CTA style.
miina Nov 22, 2018
81a7459
Add outline for CTA layer blocks, too.
miina Nov 22, 2018
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
27 changes: 24 additions & 3 deletions blocks/amp-story/amp-story-cta-layer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,8 @@ const {
} = wp.blocks;
const {
InspectorControls,
InnerBlocks
InnerBlocks,
RichText
jacobschweitzer marked this conversation as resolved.
Show resolved Hide resolved
} = wp.editor;
const {
Notice,
Expand Down Expand Up @@ -47,6 +48,10 @@ const ALLOWED_BLOCKS = [
'core/video'
];

const TEMPLATE = [
[ 'RichText', { placeholder: 'Add CTA here' } ],
];

/**
* Register block.
*/
Expand Down Expand Up @@ -105,9 +110,25 @@ export default registerBlockType(
}

componentDidMount() {
const rootClientID = getBlockRootClientId( this.props.clientId );
const blockIndex = wp.data.select( 'core/editor' ).getBlockIndex( rootClientID );
let noticeMessage = null;
const noticeOptions = {
id: 'amp-errors-notice-removed-cta'
};

if ( this.props.attributes.hasMultipleCtaBlocks ) {
removeBlock( this.props.clientId );
dispatch( 'core/editor' ).createWarningNotice( __( 'Multiple CTA Layers are not allowed, the block was removed.', 'amp' ) );
noticeMessage = wp.i18n.__( 'Multiple CTA Layers are not allowed, the block was removed.', 'amp' );
}

if ( 0 === blockIndex ) {
jacobschweitzer marked this conversation as resolved.
Show resolved Hide resolved
removeBlock( this.props.clientId );
noticeMessage = wp.i18n.__( 'CTA layer is not allowed on the first page, the block was removed.', 'amp' );
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually CTA Layer is allowed on each page -- it has to be the last Layer of a Page, so it can't be the first Layer in case there is another Layer already existing.

Copy link
Contributor

Choose a reason for hiding this comment

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

For reference: https://www.ampproject.org/docs/reference/components/amp-story#amp-story-cta-layer

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 yeah that is interesting, I wonder why I got that console error then. I'll test again and see what happens.

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 tried again and got the same error, it is the last layer on the first page:
image

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, looks like they have updated AMP but didn't update the docs accordingly.

Thanks for finding and adding this!

Probably it would be good to hide the CTA Layer Icon via CSS from the Inserter of the first Page then, too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mehigh Could you look into this CSS update? We want to hide the CTA layer button from the first story page.

}

if ( noticeMessage ) {
dispatch( 'core/notices' ).createNotice( 'warning', noticeMessage, noticeOptions );
}
}

Expand Down Expand Up @@ -137,7 +158,7 @@ export default registerBlockType(
}
</PanelBody>
</InspectorControls>,
<InnerBlocks key='contents' allowedBlocks={ ALLOWED_BLOCKS } />
<InnerBlocks key='contents' allowedBlocks={ ALLOWED_BLOCKS } template={ TEMPLATE } />
];
}

Expand Down
8 changes: 8 additions & 0 deletions blocks/amp-story/block-selector.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,14 @@ class BlockSelector extends Component {
let className = 'component-editor__selector template-' + template;
if ( isBlockSelected( block.clientId ) || hasSelectedInnerBlock( block.clientId ) ) {
className += ' is-selected';

// Do not blur the CTA layer so the user can edit the elements within it.
if ( 'amp/amp-story-cta-layer' === block.name ) {
const ampStoryDiv = document.getElementById( 'block-' + block.clientId );
if ( ampStoryDiv ) {
ampStoryDiv.classList.add( 'is-selected' );
Copy link
Contributor

Choose a reason for hiding this comment

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

@jacobschweitzer What seems to be the issue here with blurring? Perhaps that's something that @mehigh can take a look at within CSS instead? CTA Layer shouldn't probably be blurred at any point since it's always the top layer -- so it wouldn't be possible to select a layer that's on top of CTA Layer.

Mike, do you think you could look into CTA Layer to make sure that it's not being blurred when edited? There seems to be another new CSS-related issue with CTA Layer -- The CTA Layer should always take 20% of the Page height, however, it looks like the CTA Layer has the same height as the other layers:

screen shot 2018-11-22 at 9 25 59 am

Copy link
Contributor

Choose a reason for hiding this comment

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

Also note that the Layer Cards ( block-selector.js ) will be removed within #1630: that's something that will not exist soon and will be replaced by the Block Navigator on the left.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@miina Here is what I was seeing with the blurring issue:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The underlying issue seems to be that the child block gets the is-selected class so it is not blurry, but its parent (div.amp-story-cta-layer) does not have is-selected so it is blurred which makes all its child elements blurred.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jacobschweitzer In the helpers.js file there is the method maybeIsSelectedParentClass which is used for adding is-selected-parent class to page and also layers -- except for CTA Layer -- perhaps you could implement this for the CTA Layer as well? This might fix the issue and if it does not right away, it would give a CSS selector for fixing the issue via CSS, as it's done for the other layers at this moment. Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok thanks @miina that looks like a good approach, I'll start working on that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@miina I've got it working consistently after this commit - 5211097
Thanks @mehigh 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

@jacobschweitzer As mentioned above then block-selector.js file is going to be removed. Could you move adding this class to the file amp-story-editor-blocks.js where this class is being added to the other layer blocks as well?

Copy link
Contributor

Choose a reason for hiding this comment

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

For reference: https://github.com/Automattic/amp-wp/blob/amp-stories/assets/js/amp-story-editor-blocks.js#L228-L230

}
}
}

let blockType = getBlockType( block.name );
Expand Down