Carousel Block #143

jeffersonrabb · 2019-10-08T04:17:50Z

All Submissions:

Have you followed the Newspack Contributing guideline?
Does your code follow the WordPress' coding standards and VIP Go coding standards?
Have you checked to ensure there aren't other open Pull Requests for the same update/change?

Changes proposed in this Pull Request:

Adds a Carousel block that uses amp-carousel to display posts. amp-carousel cannot currently be used in the editor because it does not support DOM mutation after page load, which occurs whenever the array of posts to be displayed changes. Eventually Bento AMP will resolve this, but until this is available we will use the Swiper library to make the block functional in the editor. Currently, there is only a placeholder in the editor.

Closes #48.

How to test the changes in this Pull Request:

Check out the branch and execute npm run build:webpack
Add Articles Carousel block to a post.

Other information:

Have you added an explanation of what your changes do and why you'd like us to include them?
Have you written new tests for your changes, as applicable?
Have you successfully ran tests with your changes locally?

thomasguillot · 2019-10-09T09:31:34Z

Any chance we can have some settings for the autoplay like for Jetpack Slideshow? (and maybe the effect).

https://cld.wthms.co/tjUxYc

jeffersonrabb · 2019-10-09T09:40:39Z

Any chance we can have some settings for the autoplay like for Jetpack Slideshow? (and maybe the effect).

We can support the toggle and the interval, but AFAIK amp-carousel does not support alternative transitions. Docs are here. See anything I missed?

thomasguillot · 2019-10-09T09:44:26Z

We can support the toggle and the interval, but AFAIK amp-carousel does not support alternative transitions. Docs are here. See anything I missed?

Ok, no problem for the transitions. The autoplay is much more needed 😊

jeffersonrabb · 2019-10-09T09:56:27Z

The autoplay is much more needed

Added in 07a9db4

thomasguillot · 2019-10-09T11:31:25Z

Added in 07a9db4

That was quick! 😊
Can we have a play/pause button when using the autoplay?

thomasguillot · 2019-10-09T13:50:36Z

@jeffersonrabb I committed a bunch of fixes:

Update block icon
Resize avatar size on > tablet
Have links in white for better contrast against black
Make sure .entry-wrapper doesn't overflow
Add a cursor: pointer to the prev/next buttons

How about we add a new style? Something like this:

Can we have 2 toggles (on by default) to:

show/hide the bullets, and
show/hide the arrows

jeffersonrabb · 2019-10-09T19:36:04Z

Can we have a play/pause button when using the autoplay?

Added in ~~07a9db4~~

Correction: ce71688

@thomasguillot I left the play/pause button in the upper left corner, for you to position however you see fit.

jeffersonrabb · 2019-10-09T19:49:28Z

show/hide the bullets, and show/hide the arrows

Wouldn't hiding these be compromising the block's usability and accessibility?

jeffersonrabb · 2019-10-09T19:50:14Z

How about we add a new style?

I like the alternate style. Another possible style might be to use thumbnails instead of bullets?

thomasguillot · 2019-10-10T08:02:52Z

Wouldn't hiding these be compromising the block's usability and accessibility?

Yes and no 😊

The arrows are technically not needed since you can navigate with the bullets.
If you hide the bullets, a visitor will have no idea how many slides there are or which one is on. Maybe we could have a counter if the bullets are hidden, like "2 / 5".
You can only hide both if you are on autoplay (otherwise how would a visitor see the rest of the carousel 😅). But yes, this option isn't great for usability and accessibility. Maybe we could display a warning message.

I think allowing these to be hidden offers a bit more flexibility to the block and front-end design.

thomasguillot · 2019-10-10T08:03:21Z

Another possible style might be to use thumbnails instead of bullets?

Thumbnails would be great!

thomasguillot · 2019-10-10T09:11:18Z

In b981df6 I added a few more changes:

I moved the play/pause button to the top right corner like for Jetpack Slideshow.
I increased the maximum delay time to 20 s because I felt that 5 s was not always enough time to process, the image, the title, the author and the date.
And I set the default time to 5 s rather than 3 s.

thomasguillot · 2019-10-10T11:47:15Z

One thing I noticed is if you completely disable the AMP plugin, there's no fallback and you get all the posts, one after the other.

westonruter · 2019-10-10T16:27:54Z

src/blocks/carousel/view.php

+		wp_enqueue_script(
+			'amp-carousel',
+			'https://cdn.ampproject.org/v0/amp-carousel-0.1.js',
+			null,
+			NEWSPACK_BLOCKS__VERSION,
+			true
+		);
+		wp_enqueue_script(
+			'amp-selector',
+			'https://cdn.ampproject.org/v0/amp-selector-0.1.js',
+			null,
+			NEWSPACK_BLOCKS__VERSION,
+			true
+		);


The AMP plugin is already registering this script in amp_register_default_scripts(): https://github.com/ampproject/amp-wp/blob/039554da947563bd7a3432aa724282b1c20a86b9/includes/amp-helper-functions.php#L435-L499

These scripts are registered even when the page is not AMP. They are registered always. It gets amp-runtime as a dependency.

So no script is registered here.

So all you need to do here is:

wp_enqueue_script( 'amp-carousel' ); wp_enqueue_script( 'amp-selector' );

In reality, you can even remove the Newspack_Blocks::is_amp() check because enqueueing those scripts will not hurt in AMP pages. The AMP plugin will ensure that those scripts are present if they weren't already enqueued when there is an amp-carousel or amp-selector in the page.

The AMP plugin is already registering this script in amp_register_default_scripts():

What if the AMP plugin is not installed at all?

jeffersonrabb · 2019-10-10T17:21:10Z

One thing I noticed is if you completely disable the AMP plugin, there's no fallback and you get all the posts, one after the other.

Should be resolved in b2c2a5b

jeffersonrabb · 2019-10-11T04:06:34Z

Editor implementation using Swiper: 5dc0ddb

thomasguillot · 2019-10-11T09:02:42Z

I added a few changes to the editor's stylesheet in 4f4523c

* Have links in white for better contrast against black * Make sure `.entry-wrapper` doesn't overflow * Add a `cursor: pointer` to the prev/next buttons

* Reposition the play/pause button to the top right corner * Increase maximum delay time to 20 s * Set default delay time to 5 s

…tar.

…icles blocks

claudiulodro

Looks good, just needs some minor tweaks. I'll make those updates on Monday and then this should be good to go.

src/blocks/carousel/edit.js

src/blocks/carousel/view.php

src/blocks/carousel/edit.js

src/blocks/carousel/view.php

claudiulodro

This should be good to go. We can fine-tune the customization options in future PRs if needed.

matticbot · 2020-03-02T18:14:33Z

🎉 This PR is included in version 1.0.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

jeffersonrabb requested a review from thomasguillot October 8, 2019 04:20

jeffersonrabb added the [Status] Needs Design Review label Oct 8, 2019

westonruter mentioned this pull request Oct 8, 2019

Add caption support to the Gallery block in <amp-carousel> and fix displaying Gallery block as carousel in WP 5.3 ampproject/amp-wp#3285

Merged

westonruter reviewed Oct 10, 2019

View reviewed changes

jeffersonrabb and others added 10 commits October 11, 2019 07:29

Carousel block, initial commit.

3e1ed33

More descriptive placeholder in editor.

4883058

Autoplay and delay controls.

cd3fa6f

Update block icon

2ab811d

Resize avatar size on > tablet

eccd27d

Multiple fixes

8465bfe

* Have links in white for better contrast against black * Make sure `.entry-wrapper` doesn't overflow * Add a `cursor: pointer` to the prev/next buttons

Support for adding arbitrary classes to server side rendered blocks.

39ae205

Autoplay pause/play UI.

ad4107c

Multiple fixes

d5168b2

* Reposition the play/pause button to the top right corner * Increase maximum delay time to 20 s * Set default delay time to 5 s

Enqueue AMP scripts if AMP plugin is not installed.

301d8f4

jeffersonrabb and others added 5 commits October 11, 2019 07:29

Use Swiper in the editor to approximate view display and interactivity.

7b3cf2f

Reposition buttons for the editor stylesheet

4adf4d1

Limit the height of the Entry Title to 3 lines of text

a3895ff

Add category display.

8b0f596

Article meta data visibility settings for Date, Category, Author, Ava…

14d3921

…tar.

jeffersonrabb force-pushed the add/carousel branch from 14f178b to 14d3921 Compare October 11, 2019 11:55

thomasguillot added 2 commits October 11, 2019 13:51

Display prev/next buttons only on larger screens

86973a9

Add style for the Categories

ed461e9

jeffersonrabb added the [Status] Needs Review label Oct 11, 2019

jeffersonrabb requested a review from claudiulodro October 11, 2019 12:58

jeffersonrabb removed the [Status] Needs Design Review label Oct 11, 2019

thomasguillot added 2 commits October 11, 2019 16:06

Add class to categories and reset its style

56fde58

Move the Categories above the Entry Title, just like the Homepage Art…

71a3759

…icles blocks

claudiulodro self-assigned this Oct 13, 2019

claudiulodro requested changes Oct 14, 2019

View reviewed changes

claudiulodro added Needs Changes or Feedback and removed [Status] Needs Review labels Oct 14, 2019

claudiulodro added 4 commits October 14, 2019 11:27

class -> className

262f2d1

Skip slides with no images in preview

4558642

Improve query for posts with images

003741c

Make carousel use autocomplete searches

91ca6a8

claudiulodro approved these changes Oct 14, 2019

View reviewed changes

claudiulodro added [Status] Approved and removed Needs Changes or Feedback labels Oct 14, 2019

claudiulodro merged commit f3a72e1 into master Oct 14, 2019

claudiulodro deleted the add/carousel branch October 14, 2019 22:42

matticbot added the released label Mar 2, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Carousel Block #143

Carousel Block #143

jeffersonrabb commented Oct 8, 2019 •

edited

Loading

thomasguillot commented Oct 9, 2019

jeffersonrabb commented Oct 9, 2019

thomasguillot commented Oct 9, 2019

jeffersonrabb commented Oct 9, 2019

thomasguillot commented Oct 9, 2019

thomasguillot commented Oct 9, 2019

jeffersonrabb commented Oct 9, 2019 •

edited

Loading

jeffersonrabb commented Oct 9, 2019

jeffersonrabb commented Oct 9, 2019

thomasguillot commented Oct 10, 2019

thomasguillot commented Oct 10, 2019

thomasguillot commented Oct 10, 2019

thomasguillot commented Oct 10, 2019

westonruter Oct 10, 2019

jeffersonrabb Oct 10, 2019

jeffersonrabb commented Oct 10, 2019

jeffersonrabb commented Oct 11, 2019

thomasguillot commented Oct 11, 2019

claudiulodro left a comment

claudiulodro left a comment

matticbot commented Mar 2, 2020

Carousel Block #143

Carousel Block #143

Conversation

jeffersonrabb commented Oct 8, 2019 • edited Loading

All Submissions:

Changes proposed in this Pull Request:

How to test the changes in this Pull Request:

Other information:

thomasguillot commented Oct 9, 2019

jeffersonrabb commented Oct 9, 2019

thomasguillot commented Oct 9, 2019

jeffersonrabb commented Oct 9, 2019

thomasguillot commented Oct 9, 2019

thomasguillot commented Oct 9, 2019

jeffersonrabb commented Oct 9, 2019 • edited Loading

jeffersonrabb commented Oct 9, 2019

jeffersonrabb commented Oct 9, 2019

thomasguillot commented Oct 10, 2019

thomasguillot commented Oct 10, 2019

thomasguillot commented Oct 10, 2019

thomasguillot commented Oct 10, 2019

westonruter Oct 10, 2019

Choose a reason for hiding this comment

jeffersonrabb Oct 10, 2019

Choose a reason for hiding this comment

jeffersonrabb commented Oct 10, 2019

jeffersonrabb commented Oct 11, 2019

thomasguillot commented Oct 11, 2019

claudiulodro left a comment

Choose a reason for hiding this comment

claudiulodro left a comment

Choose a reason for hiding this comment

matticbot commented Mar 2, 2020

jeffersonrabb commented Oct 8, 2019 •

edited

Loading

jeffersonrabb commented Oct 9, 2019 •

edited

Loading