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.

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

[Gutenberg] Add AMP Carousel for Gallery and AMP Lightbox features for Gallery and Image #1121

Merged
merged 33 commits into from
May 30, 2018
Merged
Show file tree
Hide file tree
Changes from 21 commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
52943ed
Start adding carousel toggle for Gallery.
miina May 4, 2018
fd662d7
Merge #1008.
miina May 5, 2018
54f9555
Add amp-carousel class.
miina May 5, 2018
ed1f9de
Filter saving the post for when AMP Plugin gets disabled. Add tests.
miina May 5, 2018
0263878
Add lightbox logic for Image block.
miina May 7, 2018
40486fe
Add lightbox for gallery block.
miina May 8, 2018
96bd515
Fix CSS.
miina May 8, 2018
553c4da
Merge develop.
miina May 8, 2018
22060ec
Add block-related JS with blocks filter instead of admin filter.
miina May 8, 2018
5b63476
Fix issue with assigning props directly.
miina May 9, 2018
2e102ea
Add lightbox to gallery shortcode block.
miina May 9, 2018
c3aad13
Merge remote-tracking branch 'origin/develop' into add/amp-carousel-t…
miina May 16, 2018
dd924ec
Merge #1026
miina May 16, 2018
ca133a4
Adjust logic to using data- attributes.
miina May 16, 2018
22597fa
Fixes. Merge #1008
miina May 21, 2018
b70af66
Update method comments. Remove unused dynamicBlocks data.
miina May 21, 2018
438567d
Merge branch 'develop' of https://github.com/Automattic/amp-wp into a…
westonruter May 21, 2018
7ee5fd3
Merge branch 'develop' into add/amp-carousel-toggle
westonruter May 23, 2018
58430e4
Merge develop & resolve conflicts.
miina May 25, 2018
55e8fff
Resolve conflicts.
miina May 25, 2018
f658c43
Merge remote-tracking branch 'origin/develop' into add/amp-carousel-t…
miina May 28, 2018
6900e2b
Fixes.
miina May 28, 2018
a69dd55
Merge develop & resolve conflicts.
miina May 30, 2018
ff2cdd2
Merge develop.
miina May 30, 2018
d5169ef
Fixes after merge.
miina May 30, 2018
13267f6
Fix displaying AMP carousel.
miina May 30, 2018
134335e
Fix checking for shortcode.
miina May 30, 2018
26c491e
Fix jscs.
miina May 30, 2018
46c3a50
Merge branch 'develop' of https://github.com/Automattic/amp-wp into a…
westonruter May 30, 2018
201654f
Fix AMP carousel for gallery block.
westonruter May 30, 2018
f9f3a05
Pass through all attributes from img to amp-img
westonruter May 30, 2018
cc53173
Force carousels to be used when AMP theme support is absent (for back…
westonruter May 30, 2018
dbe281c
Remove apparently unneessary object-fit from amp-image-lightbox; remo…
westonruter May 30, 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
4 changes: 4 additions & 0 deletions assets/css/amp-default.css
Original file line number Diff line number Diff line change
Expand Up @@ -9,3 +9,7 @@
/** Force the image into a box of fixed dimensions and use object-fit to scale. **/
object-fit: contain;
}

#amp-image-lightbox-1 img {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should be targeting img elements in selectors. I should think that only targeting amp-img should be done since img should really be the domain of the amp-img to control?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like amp-image-lightbox component is empty by default and clicking on image which is targeting this lightbox will insert the img element into the amp-image-lightbox, the amp-image-lightbox component doesn't include an additional amp-img component within, targeting amp-img doesn't seem to be possible in this case.
For example after clicking on an an image the lightbox HTML looks like this:
screen shot 2018-05-21 at 10 34 06 pm
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.

Changed to use AMP-generated class instead of img within 6900e2b.

object-fit: contain;
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't seem to be having any effect. I don't think it is needed?

}
256 changes: 203 additions & 53 deletions assets/js/amp-editor-blocks.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
/* exported ampEditorBlocks */
/* eslint no-magic-numbers: [ "error", { "ignore": [ 1, -1, 0 ] } ] */

var __ = wp.i18n.__;

var ampEditorBlocks = ( function() {
var component = {

Expand All @@ -10,22 +12,23 @@ var ampEditorBlocks = ( function() {
data: {
dynamicBlocks: [],
ampLayoutOptions: [
{ value: 'nodisplay', label: 'No Display' },
{ value: 'fixed', label: 'Fixed' }, // Not supported by amp-audio and amp-pixel.
{ value: 'responsive', label: 'Responsive' }, // To ensure your AMP element displays, you must specify a width and height for the containing element.
{ value: 'fixed-height', label: 'Fixed height' },
{ value: 'fill', label: 'Fill' },
{ value: 'container', label: 'Container' }, // Not supported by img and video.
{ value: 'flex-item', label: 'Flex Item' },
{ value: 'intrinsic', label: 'Intrinsic' } // Not supported by video.
{ value: 'nodisplay', label: __( 'No Display' ) },
{ value: 'fixed', label: __( 'Fixed' ) }, // Not supported by amp-audio and amp-pixel.
{ value: 'responsive', label: __( 'Responsive' ) }, // To ensure your AMP element displays, you must specify a width and height for the containing element.
{ value: 'fixed-height', label: __( 'Fixed height' ) },
{ value: 'fill', label: __( 'Fill' ) },
{ value: 'container', label: __( 'Container' ) }, // Not supported by img and video.
{ value: 'flex-item', label: __( 'Flex Item' ) },
{ value: 'intrinsic', label: __( 'Intrinsic' ) } // Not supported by video.
],
defaultWidth: 608, // Max-width in the editor.
defaultHeight: 400,
mediaBlocks: [
'core/image',
'core/video',
'core/audio'
]
],
ampPanelLabel: __( 'AMP Settings' )
}
};

Expand Down Expand Up @@ -101,8 +104,8 @@ var ampEditorBlocks = ( function() {
* @return {*} Settings.
*/
component.addAMPAttributes = function addAMPAttributes( settings, name ) {
// Gallery settings for shortcode.
if ( 'core/shortcode' === name ) {
// AMP Carousel settings.
if ( 'core/shortcode' === name || 'core/gallery' === name ) {
if ( ! settings.attributes ) {
settings.attributes = {};
}
Expand All @@ -111,6 +114,16 @@ var ampEditorBlocks = ( function() {
};
}

// Add AMP Lightbox settings.
if ( 'core/gallery' === name || 'core/image' === name ) {
if ( ! settings.attributes ) {
settings.attributes = {};
}
settings.attributes.ampLightbox = {
type: 'boolean'
};
}

// Layout settings for embeds and media blocks.
if ( 0 === name.indexOf( 'core-embed' ) || -1 !== component.data.mediaBlocks.indexOf( name ) ) {
if ( ! settings.attributes ) {
Expand Down Expand Up @@ -163,6 +176,10 @@ var ampEditorBlocks = ( function() {
}, props ) )
];
}
} else if ( 'core/gallery' === name ) {
inspectorControls = component.setUpGalleryInpsectorControls( props );
} else if ( 'core/image' === name ) {
inspectorControls = component.setUpImageInpsectorControls( props );
} else if ( -1 !== component.data.mediaBlocks.indexOf( name ) || 0 === name.indexOf( 'core-embed/' ) ) {
inspectorControls = component.setUpInspectorControls( props );
}
Expand Down Expand Up @@ -223,45 +240,162 @@ var ampEditorBlocks = ( function() {
};

/**
* Default setup for inspector controls.
* Get AMP Layout select control.
*
* @param {Object} props Props.
* @return {Object|Element|*|{$$typeof, type, key, ref, props, _owner}} Inspector Controls.
* @return {Object} Element.
*/
component.setUpInspectorControls = function setUpInspectorControls( props ) {
component.getAmpLayoutControl = function getAmpLayoutControl( props ) {
var ampLayout = props.attributes.ampLayout,
ampNoLoading = props.attributes.ampNoLoading,
isSelected = props.isSelected,
name = props.name,
el = wp.element.createElement,
InspectorControls = wp.blocks.InspectorControls,
SelectControl = wp.components.SelectControl,
ToggleControl = wp.components.ToggleControl,
PanelBody = wp.components.PanelBody,
label = 'AMP Layout';
name = props.name,
label = __( 'AMP Layout' );

if ( 'core/image' === name ) {
label = 'AMP Layout (modifies width/height)';
label = __( 'AMP Layout (modifies width/height)' );
}

return el( SelectControl, {
label: label,
value: ampLayout,
options: component.getLayoutOptions( name ),
onChange: function( value ) {
props.setAttributes( { ampLayout: value } );
}
} );
};

/**
* Get AMP Noloading toggle control.
*
* @param {Object} props Props.
* @return {Object} Element.
*/
component.getAmpNoloadingToggle = function getAmpNoloadingToggle( props ) {
var ampNoLoading = props.attributes.ampNoLoading,
el = wp.element.createElement,
ToggleControl = wp.components.ToggleControl,
label = __( 'AMP Noloading' );

return el( ToggleControl, {
label: label,
checked: ampNoLoading,
onChange: function() {
props.setAttributes( { ampNoLoading: ! ampNoLoading } );
}
} );
};

/**
* Get AMP Lightbox toggle control.
*
* @param {Object} props Props.
* @return {Object} Element.
*/
component.getAmpLightboxToggle = function getAmpLightboxToggle( props ) {
var ampLightbox = props.attributes.ampLightbox,
el = wp.element.createElement,
ToggleControl = wp.components.ToggleControl,
label = __( 'Add lightbox effect' );

return el( ToggleControl, {
label: label,
checked: ampLightbox,
onChange: function( nextValue ) {
props.setAttributes( { ampLightbox: ! ampLightbox } );
// In case of lightbox set linking images to 'none'.
if ( nextValue && props.attributes.linkTo && 'none' !== props.attributes.linkTo ) {
props.setAttributes( { linkTo: 'none' } );
}
}
} );
};

/**
* Get AMP Carousel toggle control.
*
* @param {Object} props Props.
* @return {Object} Element.
*/
component.getAmpCarouselToggle = function getAmpCarouselToggle( props ) {
var ampCarousel = props.attributes.ampCarousel,
el = wp.element.createElement,
ToggleControl = wp.components.ToggleControl,
label = __( 'Display as AMP carousel' );

return el( ToggleControl, {
label: label,
checked: ampCarousel,
onChange: function() {
props.setAttributes( { ampCarousel: ! ampCarousel } );
}
} );
};

/**
* Default setup for inspector controls.
*
* @param {Object} props Props.
* @return {Object|Element|*|{$$typeof, type, key, ref, props, _owner}} Inspector Controls.
*/
component.setUpInspectorControls = function setUpInspectorControls( props ) {
var isSelected = props.isSelected,
el = wp.element.createElement,
InspectorControls = wp.blocks.InspectorControls,
PanelBody = wp.components.PanelBody;

return isSelected && (
el( InspectorControls, { key: 'inspector' },
el( PanelBody, { title: component.data.ampPanelLabel },
component.getAmpLayoutControl( props ),
component.getAmpNoloadingToggle( props )
)
)
);
};

/**
* Set up inspector controls for Image block.
*
* @param {Object} props Props.
* @return {Object} Inspector Controls.
*/
component.setUpImageInpsectorControls = function setUpImageInpsectorControls( props ) {
var isSelected = props.isSelected,
el = wp.element.createElement,
InspectorControls = wp.blocks.InspectorControls,
PanelBody = wp.components.PanelBody;

return isSelected && (
el( InspectorControls, { key: 'inspector' },
el( PanelBody, { title: component.data.ampPanelLabel },
component.getAmpLayoutControl( props ),
component.getAmpNoloadingToggle( props ),
component.getAmpLightboxToggle( props )
)
)
);
};

/**
* Set up inspector controls for Gallery block.
* Adds ampCarousel attribute for displaying the output as amp-carousel.
*
* @param {Object} props Props.
* @return {*} Inspector controls.
*/
component.setUpGalleryInpsectorControls = function setUpGalleryInpsectorControls( props ) {
var isSelected = props.isSelected,
el = wp.element.createElement,
InspectorControls = wp.blocks.InspectorControls,
PanelBody = wp.components.PanelBody;

return isSelected && (
el( InspectorControls, { key: 'inspector' },
el( PanelBody, { title: 'AMP Settings' },
el( SelectControl, {
label: label,
value: ampLayout,
options: component.getLayoutOptions( name ),
onChange: function( value ) {
props.setAttributes( { ampLayout: value } );
}
} ),
el( ToggleControl, {
label: 'AMP Noloading',
checked: ampNoLoading,
onChange: function() {
props.setAttributes( { ampNoLoading: ! ampNoLoading } );
}
} )
el( PanelBody, { title: component.data.ampPanelLabel },
component.getAmpCarouselToggle( props ),
component.getAmpLightboxToggle( props )
)
)
);
Expand All @@ -275,26 +409,16 @@ var ampEditorBlocks = ( function() {
* @return {*} Inspector controls.
*/
component.setUpShortcodeInspectorControls = function setUpShortcodeInspectorControls( props ) {
var ampCarousel = props.attributes.ampCarousel,
isSelected = props.isSelected,
var isSelected = props.isSelected,
el = wp.element.createElement,
InspectorControls = wp.blocks.InspectorControls,
ToggleControl = wp.components.ToggleControl,
PanelBody = wp.components.PanelBody,
toggleControl;
PanelBody = wp.components.PanelBody;

if ( component.isGalleryShortcode( props.attributes ) ) {
toggleControl = el( ToggleControl, {
label: 'Display as AMP carousel',
checked: ampCarousel,
onChange: function() {
props.setAttributes( { ampCarousel: ! ampCarousel } );
}
} );
return isSelected && (
el( InspectorControls, { key: 'inspector' },
el( PanelBody, { title: 'AMP Settings' },
toggleControl
el( PanelBody, { title: component.data.ampPanelLabel },
component.getAmpCarouselToggle( props )
)
)
);
Expand Down Expand Up @@ -347,13 +471,19 @@ var ampEditorBlocks = ( function() {
);
}

// In case AMP Layout, add layout to classname.
// In case AMP attributes, add info to classname.
if ( component.hasAmpLayoutSet( attributes || '' ) ) {
ampClassName += ' amp-layout-' + attributes.ampLayout;
}
if ( component.hasAmpNoLoadingSet( attributes || '' ) ) {
ampClassName += ' amp-noloading';
}
if ( component.hasAmpCarouselSet( attributes || '' ) ) {
ampClassName += ' amp-carousel';
}
if ( component.hasAmpLightboxSet( attributes || '' ) ) {
ampClassName += ' amp-lightbox';
}

if ( '' !== ampClassName && attributes.className !== ampClassName ) {
props.className = ampClassName.trim();
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @miina,
In my browser, this didn't assign the new props.className value, causing the lightbox to not appear on the front-end.

But with this change (borrowing from Weston's solution here), it did:

props = _.extend( {}, props, { className: ampClassName.trim() } );

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @kienstra, thanks for finding this, makes sense that the props.className can't be directly set this way.

Expand All @@ -365,6 +495,26 @@ var ampEditorBlocks = ( function() {
return element;
};

/**
* Check if AMP Lightbox is set.
*
* @param {Object} attributes Attributes.
* @return {boolean} If is set.
*/
component.hasAmpLightboxSet = function hasAmpLightboxSet( attributes ) {
return attributes.ampLightbox && false !== attributes.ampLightbox;
};

/**
* Check if AMP Carousel is set.
*
* @param {Object} attributes Attributes.
* @return {boolean} If is set.
*/
component.hasAmpCarouselSet = function hasAmpCarouselSet( attributes ) {
return attributes.ampCarousel && false !== attributes.ampCarousel;
};

/**
* Check if AMP NoLoading is set.
*
Expand Down
Loading