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

Update cover image markup and CSS #3444

Merged
merged 6 commits into from
Mar 13, 2018

Conversation

samikeijonen
Copy link
Contributor

@samikeijonen samikeijonen commented Nov 12, 2017

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
Copy link
Member

aduth commented Nov 14, 2017

Related: #2541

@samikeijonen
Copy link
Contributor Author

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

@afercia
Copy link
Contributor

afercia commented Dec 12, 2017

@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
Copy link
Member

aduth commented Dec 12, 2017

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 samikeijonen force-pushed the update/cover-image branch 2 times, most recently from ac92034 to aaf961d Compare January 2, 2018 11:52
@samikeijonen
Copy link
Contributor Author

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

@samikeijonen
Copy link
Contributor Author

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

Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

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

const style = url ?
{ backgroundImage: `url(${ url })` } :
undefined;
const classes = classnames(
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@aduth
Copy link
Member

aduth commented Jan 16, 2018

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
Copy link
Contributor Author

samikeijonen commented Jan 17, 2018

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

@jasmussen
Copy link
Contributor

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
Copy link
Contributor Author

samikeijonen commented Jan 23, 2018

@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
Copy link
Contributor

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

@samikeijonen
Copy link
Contributor Author

@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
Copy link
Contributor Author

Fixed merge conflict and added link color to white.

const classes = classnames(
className,
align ? `align${ align }` : null,
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like a duplicate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed duplicate and fixed merge conflict.

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

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

@samikeijonen
Copy link
Contributor Author

@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
Copy link
Contributor

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

@samikeijonen
Copy link
Contributor Author

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

@youknowriad
Copy link
Contributor

@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
Copy link
Contributor Author

Thanks for the fix!

We could add font-weight: 700.

@samikeijonen
Copy link
Contributor Author

@jasmussen Can we merge this?

@jasmussen
Copy link
Contributor

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

I got these two screenshots from the branch:

screen shot 2018-03-08 at 14 13 36

screen shot 2018-03-08 at 14 13 44

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

@samikeijonen
Copy link
Contributor Author

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
Copy link
Contributor Author

@youknowriad Can we merge this?

@jasmussen jasmussen merged commit 55c6f2c into WordPress:master Mar 13, 2018
@samikeijonen samikeijonen deleted the update/cover-image branch March 13, 2018 08:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants