Update cover image markup and CSS by samikeijonen · Pull Request #3444 · WordPress/gutenberg

samikeijonen · 2017-11-12T14:06:20Z

Description

Update cover image markup and CSS discussed in #2902.

How Has This Been Tested?

Tested with default themes and random .org themes.
Running npm test gives error. I can't seem to track down why? It's about "isValid": false,.

 File 'core__cover-image.json' does not match expected value:

    expect(received).toEqual(expected)

    Expected value to equal:
      [{"attributes": {"dimRatio": 50, "hasParallax": false, "title": ["Guten Berg!"], "url": "https://cldup.com/uuUqE_dXzy.jpg"}, "isValid": true, "name": "core/cover-image", "originalContent": "<div class=\"wp-block-cover-image has-background-dim-50 has-background-dim\" style=\"background-image:url(https://cldup.com/uuUqE_dXzy.jpg)\">
        <p class=\"wp-block-cover-image-text\">Guten Berg!</p>
    </div>", "uid": "_uid_0"}]
    Received:
      [{"attributes": {"dimRatio": 50, "hasParallax": false, "title": ["Guten Berg!"], "url": "https://cldup.com/uuUqE_dXzy.jpg"}, "isValid": false, "name": "core/cover-image", "originalContent": "<div class=\"wp-block-cover-image has-background-dim-50 has-background-dim\" style=\"background-image:url(https://cldup.com/uuUqE_dXzy.jpg)\">
        <p class=\"wp-block-cover-image-text\">Guten Berg!</p>
    </div>", "uid": "_uid_0"}]

    Difference:

    - Expected
    + Received

    @@ -6,11 +6,11 @@
            "title": Array [
              "Guten Berg!",
            ],
            "url": "https://cldup.com/uuUqE_dXzy.jpg",
          },
    -     "isValid": true,
    +     "isValid": false,

Screenshots (jpeg or gifs if applicable):

Types of changes

Change <section> to <div>.
Change hardcoded <h2> to <p>.
Add class <p class="wp-block-cover-image-text">. This was mainly for easier styling in the editor.
Note that I don't how to correctly add class wp-block-cover-image-text. I have now added it like this: <p className={ classnames( 'wp-block-cover-image-text' ) }>

Checklist:

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

aduth · 2017-11-14T21:52:47Z

Related: #2541

samikeijonen · 2017-11-16T09:26:29Z

In 96e5b504881961fc861c2142dfc0a52679c1c8c3 I added align classes mentioned in issue #3516.

afercia · 2017-12-12T16:45:26Z

@aduth when you have a chance: what is the best way to help this PR move on? The changes proposed here make a lot of sense to me.

aduth · 2017-12-12T22:05:47Z

With #3665 having been merged, we now have an upgrade path for block markup.

#3851 is another example of a migration.

If we want to avoid breakage for existing blocks, I suggest refactoring to take advantage of this new mechanism.

samikeijonen · 2018-01-02T12:06:23Z

I added deprecate version but not sure if it's working.
When running npm test I still get errors. Something about File 'core__pullquote__multi-paragraph.json' does not match expected value:

samikeijonen · 2018-01-02T12:14:27Z

Should wp-block-cover-image-text class be wp-block-cover-image__text?

aduth

Should wp-block-cover-image-text class be wp-block-cover-image__text?

The general guidelines are outlined here:

https://github.com/WordPress/gutenberg/blob/master/docs/coding-guidelines.md#naming

In the case of blocks (particularly block front-end styling), the rules haven't been quite consistent with this. Based on the generated class I think .wp-block-cover-image-text could be fine enough.

The alternative being that we don't assign a class to the paragraph and instead apply styles via an element selector (.wp-block-cover-image > p), but arguments can be made discouraging this, as it makes the markup less tolerant to deprecation changes.

Speaking of styling and deprecation, one important consideration we should have with all blocks, including the changes here, is that we should anticipate that some older blocks exist with the deprecated markup (here, the h2 tag) and therefore we ought to always keep those styles around so users' legacy blocks' don't suddenly start breaking when viewed on the front-end (cc @youknowriad ).

When running npm test I still get errors. Something about File 'core__pullquote__multi-paragraph.json' does not match expected value:

Have you tried running the npm script for regenerating fixtures?

npm run fixtures:regenerate

aduth · 2018-01-16T16:02:34Z

blocks/library/cover-image/index.js

It's possible we could DRY this up a bit by moving the common logic into a function (similar to the shared blockAttributes variable).

Only for const style? I can do the similar as function dimRatioToClass().

aduth · 2018-01-16T20:15:47Z

Speaking of styling and deprecation, one important consideration we should have with all blocks, including the changes here, is that we should anticipate that some older blocks exist with the deprecated markup (here, the h2 tag) and therefore we ought to always keep those styles around so users' legacy blocks' don't suddenly start breaking when viewed on the front-end (cc @youknowriad ).

Related: #4408, #3851 (comment)

samikeijonen · 2018-01-17T14:38:54Z

Hmmm I messed my branch when accidently deleting also this branch on local. Now I only have one commit message in here: 80a7d815d8d9b29e9a970fc7248195632e7ba178

jasmussen · 2018-01-23T07:43:07Z

Does this PR also ensure that no empty <p> tags are output if the user hasn't entered any title? It's not a blocker, but if it's easy then we should do it.

samikeijonen · 2018-01-23T14:50:25Z

@jasmussen I just tested and it does output empty <p>. I can try to look into it.

If @aduth have time to push me into right direction that would be nice.

Edit: This is what I got for title check: 8ba755d688f50517f2f6340b1714a5eeef8f0f63

youknowriad · 2018-02-02T13:48:39Z

Not sure what's left on this PR. How can we help?

samikeijonen · 2018-02-02T17:15:18Z

@youknowriad Pretty much nothing left to do if you ask me:) There seems to be some merge conflicts, I'll try to look into them over the weekend.

samikeijonen · 2018-02-03T11:48:22Z

Fixed merge conflict and added link color to white.

youknowriad · 2018-02-21T12:48:33Z

blocks/library/cover-image/index.js

This seems like a duplicate?

I removed duplicate and fixed merge conflict.

youknowriad

This looks good to me, there's a small duplicate and failing test. I'll merge once fixed

…or.scss.

samikeijonen · 2018-03-02T09:25:00Z

@youknowriad Any update on this one? There is check errors but other that should be good to go.

I'm going trough all the changes in the front-end and this popped up.

youknowriad · 2018-03-02T09:35:55Z

Actually, I was waiting for you to fix the tests before merging :). Do you want some help in doing so?

samikeijonen · 2018-03-02T09:57:03Z

Yes please:) I don't know how to fix the tests, tried couple of times but don't really understand the reasons.

youknowriad · 2018-03-02T10:21:08Z

@jasmussen Can I have a quick design review for this before merge. The text looks lighter because it's not an H2 anymore but maybe it's better this way?

samikeijonen · 2018-03-02T10:28:29Z

Thanks for the fix!

We could add font-weight: 700.

samikeijonen · 2018-03-08T12:48:29Z

@jasmussen Can we merge this?

jasmussen · 2018-03-08T13:14:29Z

Ack my apologies, I don't know how I missed the previous ping!

I got these two screenshots from the branch:

If they look right to you, then yes, this is a good look I think. 👍 👍

samikeijonen · 2018-03-08T15:13:14Z

Looks right to me, but I didn't add font-weight: 700. I'd say let's merge this now so we have correct markup. Then we can create minor design fixes if needed.

samikeijonen · 2018-03-12T17:26:22Z

@youknowriad Can we merge this?

samikeijonen force-pushed the update/cover-image branch from 2a46732 to 96e5b50 Compare November 16, 2017 09:23

samikeijonen force-pushed the update/cover-image branch 2 times, most recently from ac92034 to aaf961d Compare January 2, 2018 11:52

aduth reviewed Jan 16, 2018

View reviewed changes

samikeijonen force-pushed the update/cover-image branch from 3bac288 to fc6b77a Compare January 17, 2018 13:00

AdamSoucie mentioned this pull request Jan 19, 2018

Cover Image H2 does not appear in Table of Contents #4606

Closed

samikeijonen force-pushed the update/cover-image branch from 12970ea to 80a7d81 Compare January 20, 2018 11:25

mcsf mentioned this pull request Jan 20, 2018

Table of Contents / Outline: Treat Cover Images as H2-level headings #4616

Closed

3 tasks

samikeijonen force-pushed the update/cover-image branch from 8ba755d to 5898663 Compare February 3, 2018 11:41

youknowriad reviewed Feb 21, 2018

View reviewed changes

samikeijonen added 4 commits February 22, 2018 11:15

Bring branch again.

77cd75d

Add wp-block-cover-image-text class in post-content.json file.

46b948e

Check user have written the title.

e696170

Add white link color in the front end so thi can be removed from edit…

7d93529

…or.scss.

samikeijonen force-pushed the update/cover-image branch from 459a70e to 7d93529 Compare February 22, 2018 09:17

Remove duplicate align.

d76ce10

samikeijonen mentioned this pull request Feb 23, 2018

Cover image block markup in the front-end #2902

Closed

Fix fixtures tests for the cover image

c6dd1e9

samikeijonen mentioned this pull request Mar 2, 2018

CSS: audit the font-size and consider a bigger minimum size #5359

Closed

jasmussen merged commit 55c6f2c into WordPress:master Mar 13, 2018

samikeijonen deleted the update/cover-image branch March 13, 2018 08:10

benoitchantre mentioned this pull request Nov 22, 2018

Update styles for Cover block wprig/wprig#193

Merged

4 tasks

Conversation

samikeijonen commented Nov 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

How Has This Been Tested?

Screenshots (jpeg or gifs if applicable):

Types of changes

Checklist:

Uh oh!

aduth commented Nov 14, 2017

Uh oh!

samikeijonen commented Nov 16, 2017

Uh oh!

afercia commented Dec 12, 2017

Uh oh!

aduth commented Dec 12, 2017

Uh oh!

samikeijonen commented Jan 2, 2018

Uh oh!

samikeijonen commented Jan 2, 2018

Uh oh!

aduth left a comment

Choose a reason for hiding this comment

Uh oh!

aduth Jan 16, 2018

Choose a reason for hiding this comment

Uh oh!

samikeijonen Jan 17, 2018

Choose a reason for hiding this comment

Uh oh!

aduth commented Jan 16, 2018

Uh oh!

samikeijonen commented Jan 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasmussen commented Jan 23, 2018

Uh oh!

samikeijonen commented Jan 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

youknowriad commented Feb 2, 2018

Uh oh!

samikeijonen commented Feb 2, 2018

Uh oh!

samikeijonen commented Feb 3, 2018

Uh oh!

youknowriad Feb 21, 2018

Choose a reason for hiding this comment

Uh oh!

samikeijonen Feb 22, 2018

Choose a reason for hiding this comment

Uh oh!

youknowriad left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samikeijonen commented Mar 2, 2018

Uh oh!

youknowriad commented Mar 2, 2018

Uh oh!

samikeijonen commented Mar 2, 2018

Uh oh!

youknowriad commented Mar 2, 2018

Uh oh!

samikeijonen commented Mar 2, 2018

Uh oh!

samikeijonen commented Mar 8, 2018

Uh oh!

jasmussen commented Mar 8, 2018

Uh oh!

samikeijonen commented Mar 8, 2018

Uh oh!

samikeijonen commented Mar 12, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

samikeijonen commented Nov 12, 2017 •

edited

Loading

samikeijonen commented Jan 17, 2018 •

edited

Loading

samikeijonen commented Jan 23, 2018 •

edited

Loading

youknowriad left a comment •

edited

Loading