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

Classic block: Make <!--more--> visible #5186

Merged
merged 1 commit into from Feb 24, 2018

Conversation

Projects
None yet
5 participants
@mcsf
Contributor

mcsf commented Feb 22, 2018

Description

Fixes #4958

Adds a visual reminiscent of that found in the classic editor, but doesn't resort to an actual image for the dashed effect. Furthermore, that classic image contained an English-only string ("read more"), whereas this change contains no strings.

How Has This Been Tested?

  1. Open the classic editor, start a new post
  2. Paste the text that follows these instructions
  3. Save the draft
  4. Open in Gutenberg
Lorem ipsum dolor sit amet, ferri vidisse nam eu, ad nec copiosae mnesarchum vituperatoribus. Te brute dicunt sea, ut vis omnium menandri, ut sumo aliquam has. Eum aperiam interpretaris at, sea et recusabo expetenda, omnis tibique mea no. Pri suas partem ea, ius sonet numquam offendit cu, ad simul admodum pri. Eum cu unum choro albucius.

<!--more-->

Prima ridens denique his te, ferri illum volumus an his. Eu vel dicat homero qualisque, vitae regione deserunt vis ei. Graeci incorrupte liberavisse no mea, saepe voluptaria usu ex, vis dicant euismod id. At dolor reprimique eos, quo altera detraxit moderatius id. Quo iudico utinam eu, ad alia munere mel.

Screenshots (jpeg or gifs if applicable):

screen shot 2018-02-22 at 00 36 32

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code has proper inline documentation.

@mcsf mcsf added this to the 2.3 milestone Feb 22, 2018

@mcsf mcsf requested a review from jasmussen Feb 22, 2018

margin: 15px auto;
outline: 0;
cursor: default;
border: 2px dashed rgb( 186, 186, 186 );

This comment has been minimized.

@aduth

aduth Feb 22, 2018

Member

Where did this color come from? Should it be expressed as a hex value? Should it be one of the standard grays in our variables set?

This comment has been minimized.

@mcsf

mcsf Feb 22, 2018

Contributor

It came from the original image, just to get a sense of how switching away from the image would look, but yeah, I'd rather merge this with a proper var from our set.

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Feb 22, 2018

Nice.

Hard to test though, if I manually paste the following into text mode, I get a classic block, a more block, and a classic block:

<p>Lorem ipsum dolor sit amet, ferri vidisse nam eu, ad nec copiosae mnesarchum vituperatoribus. Te brute dicunt sea, ut vis omnium menandri, ut sumo aliquam has. Eum aperiam interpretaris at, sea et recusabo expetenda, omnis tibique mea no. Pri suas partem ea, ius sonet numquam offendit cu, ad simul admodum pri. Eum cu unum choro albucius.</p>

<!--more-->

<p>Prima ridens denique his te, ferri illum volumus an his. Eu vel dicat homero qualisque, vitae regione deserunt vis ei. Graeci incorrupte liberavisse no mea, saepe voluptaria usu ex, vis dicant euismod id. At dolor reprimique eos, quo altera detraxit moderatius id. Quo iudico utinam eu, ad alia munere mel.</p>

It would be nice if the "Read More" label was still there. Is it because we can't make it translatable that it's gone?

@mcsf

This comment has been minimized.

Contributor

mcsf commented Feb 22, 2018

Hard to test though, if I manually paste the following into text mode, I get a classic block, a more block, and a classic block:

Apologies for the missing testing instructions — it was a commit-and-sleep evening. I'll update the description.

It would be nice if the "Read More" label was still there. Is it because we can't make it translatable that it's gone?

Agree that it would be preferable, and it would allow a clearer distinction between more and nextpage. Correct, I didn't just reuse the image because the English string is a part of it. I mean, we can get fancy with a label and some more CSS.

@mcsf

This comment has been minimized.

Contributor

mcsf commented Feb 22, 2018

It would be nice if the "Read More" label was still there. Is it because we can't make it translatable that it's gone?

Oh, and just to be super clear for anyone, this only affects <!--more--> within a Classic block; the native Gutenberg More block remains visible and can show custom labels, obviously.

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Feb 23, 2018

I was finally able to reproduce, using these steps:

  1. Paste this in text mode:

Lorem ipsum dolor sit amet, ferri vidisse nam eu, ad nec copiosae mnesarchum vituperatoribus. Te brute dicunt sea, ut vis omnium menandri, ut sumo aliquam has. Eum aperiam interpretaris at, sea et recusabo expetenda, omnis tibique mea no. Pri suas partem ea, ius sonet numquam offendit cu, ad simul admodum pri. Eum cu unum choro albucius.

  1. Switch to visual editor

  2. Click block ellipsis menu on classic block, pick "edit as HTML"

  3. Paste in this:

Lorem ipsum dolor sit amet, ferri vidisse nam eu, ad nec copiosae mnesarchum vituperatoribus. Te brute dicunt sea, ut vis omnium menandri, ut sumo aliquam has. Eum aperiam interpretaris at, sea et recusabo expetenda, omnis tibique mea no. Pri suas partem ea, ius sonet numquam offendit cu, ad simul admodum pri. Eum cu unum choro albucius.

Prima ridens denique his te, ferri illum volumus an his. Eu vel dicat homero qualisque, vitae regione deserunt vis ei. Graeci incorrupte liberavisse no mea, saepe voluptaria usu ex, vis dicant euismod id. At dolor reprimique eos, quo altera detraxit moderatius id. Quo iudico utinam eu, ad alia munere mel.

  1. Click Edit Visually in block ellipsis menu.

I now see what you see, and it's definitely a big improvement from the old classic editors image.

However I do think we should add a label there, if we can, to ambiguate between nextpage also.

If that's difficult to do, however, perhaps we shouldn't put too much effort into this at all, and keep the old image as it was. Keep in mind we have #4926 that seeks to make the Classic block look even more like the classic editor.

@mcsf

This comment has been minimized.

Contributor

mcsf commented Feb 23, 2018

@jasmussen, I agree we should keep this low-effort.

perhaps we shouldn't put too much effort into this at all, and keep the old image as it was

We can do that, yes. Are you OK with duplicating the image (from /wp-includes/js/tinymce/skins/wordpress/images/more-2x.png) into the Gutenberg plugin? The alternative would be to reference it from within the plugin's style sheets (perhaps with a disconcertingly long relative path like url( ../../../../../wp-includes/js/… )), which sounds like a big no-no.

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Feb 23, 2018

It does seem like we should duplicate it. CC: @iseulde for the expert opinion on the cleanest approach here.

@mtias

This comment has been minimized.

Contributor

mtias commented Feb 24, 2018

Let's get this in as is just to get an indication, we can figure out updates later. I think it's better if we don't load any image here.

@mcsf mcsf merged commit 7094ef5 into master Feb 24, 2018

2 checks passed

codecov/project 34.2% remains the same compared to 38e9503
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@mcsf mcsf deleted the add/classic-more-style branch Feb 24, 2018

@iseulde

This comment has been minimized.

Member

iseulde commented Feb 26, 2018

Thanks for the fix! This is also an issue in core with the image... The annoying things is that images cannot have the before or after pseudo element, so we cannot really display text that way. We could swap this to HR, which is what was proposed for core, but that does change the behaviour a bit etc. Since this is here for backward compatibly, I would just do what we currently do in core and move over the image. In general I think we should make the classic block behave as much as possible as the editor in 4.9 and only change something if there are bad bugs with it.

@mcsf

This comment has been minimized.

Contributor

mcsf commented Feb 26, 2018

The annoying things is that images cannot have the before or after pseudo element, so we cannot really display text that way. We could swap this to HR, which is what was proposed for core

Interesting that core followed the same path. While working on this PR I reached the same conclusion about img and hr but didn't like how it would entail deviating from the core editor.

Since this is here for backward compatibly, I would just do what we currently do in core and move over the image.

Sounds acceptable. I merged this one as-is in order to unblock other PRs, but we can iterate.

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