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

[Fix] Establish consistency between front-end and back-end classes for Youtube (Embed) block #10411

Merged
merged 1 commit into from Oct 15, 2018

Conversation

Projects
None yet
6 participants
@nfmohit-wpmudev
Contributor

nfmohit-wpmudev commented Oct 8, 2018

Description

This PR closes #10121 which reports the usage of is-video class in the editor for the Youtube (Embed) block, whereas is-type-video class is used for the same in the front-end. This changes the editor class from is-video to is-type-video.

How has this been tested?

This PR has been tested by going through the following steps:

  1. Started a new post using the Gutenberg editor.
  2. Added the "Youtube" (Embed) block.
  3. Using the browser developer tools, make sure the is-type-video class is used for the block in both front-end and backend instead of the is-video class.

This was tested in WP 4.9.8, Gutenberg 3.9.0, Apache server with PHP 7.2.0 and MySQL 5.6.34. According to initial tests, the code doesn’t seem to affect any other areas.

Screenshots

pull-10121

Types of changes

This PR just replaces the is-video with is-type-video class in the embed block file.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
@chrisvanpatten

This comment has been minimized.

Contributor

chrisvanpatten commented Oct 8, 2018

Is it worth continuing to support is-video with a deprecation warning or is this small enough to not matter? cc @mcsf

@mcsf

This comment has been minimized.

Contributor

mcsf commented Oct 9, 2018

I'm torn. Adding a deprecation is easy in technical terms, but they come with added maintenance cost, and I'd also like us to keep new deprecations to an essential minimum at this stage of development. Perhaps @jasmussen 's gut can estimate use of is-video? If deemed little, we could even skip the deprecation, IMO.

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Oct 10, 2018

Nice catches. To decide whether we can skip the deprecation handler, we can ask some questions:

  1. Appx. how long has the .is-video and .is-type-video classes been in release versions of the plugin?
  2. Have we had any documentation suggesting their usage?
  3. Is .is-video new to Gutenberg, or is it widely used in themes prior to Gutenberg coming along?
  4. What is the worst that can happen for a user that optimized towards .is-video, when we remove it?

My personal opinion is that we should not worry about the deprecation handler. Since my gut was asked, my gut responds that this isn't widely optimized for, and in cases where it is, it's both an easy fix and the visual regression potentially caused by its removal should be minimal.

On the flipside, the price of technical debt piling up based on deprecation handlers isn't trivial.

@mcsf

This comment has been minimized.

Contributor

mcsf commented Oct 10, 2018

Thanks for weighing in. Those questions are great, though I confess I don't have much bandwidth to investigate and answer them. Perhaps @nfmohit-wpmudev would be open to it.

@mcsf mcsf added this to the 4.1 milestone Oct 10, 2018

@nfmohit-wpmudev

This comment has been minimized.

Contributor

nfmohit-wpmudev commented Oct 14, 2018

Thank you so very much for all your concern, views and insights @chrisvanpatten, @mcsf and @jasmussen ❤️

I'll try to answer the questions that @jasmussen pointed out according to my observation and investigation:

  1. .is-video seems to have been there since the very early stages, i.e. 0.2.0 when it was introduced in #1554. On the other hand, the .is-type-* classes were introduced in #4118, when Gutenberg was 2.2.0.
  2. I wasn't able to find any documentation suggesting the usage of these classes.
  3. .is-video is not new to Gutenberg, at least according to what I can see. However, the possibility of its extensive usage in themes is very low in my opinion, as it only exists in the editor, not in the front-end.
  4. Though the possibility is low, but still even if a user might have optimized towards .is-video, the worst that can happen (in my opinion) is some of their styles targetting this specific class within the editor will stop working, keeping everything else still functional when we remove it.

In my honest opinion, I think a deprecation handler for this minor change unnecessary. .is-type-video has been there in the front-end for quite some time now and maintaining it's consistency with the editor can be considered very obvious. I stand and completely agree with your opinions @jasmussen ❤️

@mcsf

mcsf approved these changes Oct 15, 2018

Thanks so much for investigating, @nfmohit-wpmudev! I too am convinced we can skip deprecation handling. Let's get this in!

@mcsf mcsf merged commit 8aff18d into WordPress:master Oct 15, 2018

2 checks passed

codecov/project 49.29% remains the same compared to 9d419fa
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

etoledom added a commit that referenced this pull request Oct 15, 2018

Establish consistency between front-end and back-end classes for Yout…
…ube (Embed) block (#10411)

Props nfmohit-wpmudev

@mtias mtias added the [Block] Embed label Oct 18, 2018

@paaljoachim

This comment has been minimized.

paaljoachim commented Oct 22, 2018

https://make.wordpress.org/test/
14. Embed some YouTube videos and make sure they look as expected in the editor and after publishing. (10411)

Chrome newest version I think on my Macbook Pro.

Here I just pasted a Youtube link directly into a paragraph area and the video showed up...:)
I wrote caption below the video (did not click alignment options.)

Backend:
screen shot 2018-10-22 at 19 06 54

Frontend:
screen shot 2018-10-22 at 19 08 57

Alignment left backend:
screen shot 2018-10-22 at 19 10 15

Frontend:
screen shot 2018-10-22 at 19 11 03

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment