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

In blockquotes, use cite element instead of footer. #2647 #2859

Closed
wants to merge 4 commits into from

Conversation

tfrommen
Copy link
Member

@tfrommen tfrommen commented Oct 2, 2017

Description

When merged, this PR implements changes as proposed in #2647: a potential citation in a <blockquote> element is now wrapped in a <cite> tag instead of <footer>.

Changes:

  • Update source code.
  • Adapt SCSS files.
  • Adapt JS fixtures.
  • Adapt PHP fixtures.

How Has This Been Tested?

$ npm i && npm run ci
$ composer install && ./vendor/bin/phpcs
$ phpunit

Types of changes

Change: in <blockquote> elements, replace <footer> elements with <cite>.

Checklist:

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

Happy Hacktober! 🙂

@karmatosed
Copy link
Member

Can you explain why you are choosing to use cite when this is what cite html tag means:

The tag defines the title of a work (e.g. a book, a song, a movie, a TV show, a painting, a sculpture, etc.).

https://www.w3schools.com/TAgs/tag_cite.asp

@tfrommen
Copy link
Member Author

tfrommen commented Oct 2, 2017

@karmatosed it is all explained in the issue.

Also, W3Schools might not be the best place for reference (opinionated, but honest statement).

The HTML <blockquote> Element (or HTML Block Quotation Element) indicates that the enclosed text is an extended quotation. Usually, this is rendered visually by indentation (see Notes for how to change it). A URL for the source of the quotation may be given using the cite attribute, while a text representation of the source can be given using the <cite> element.

https://developer.mozilla.org/en-US/docs/Web/HTML/Element/blockquote

@mtias
Copy link
Member

mtias commented Oct 2, 2017

I believe cite was made less restrictive some time ago, but there are lingering mentions of it being restrictive.

Another point why we used footer was because we used to include a which didn't belong within the attribution. I'd be ok switching to cite if that is perfectly ok with the intention and flexibility required for this block.

I remember there were different interpretations of "text representation of the source" around for cite.

Another possibility is:

<blockquote>
    <p>Quoted text.</p>
    <footer>
        <cite>Source</cite>
   </footer>
</blockquote>

Though I'd prefer simpler markup if it retains all the semantics.

@mtias mtias added the [Feature] Blocks Overall functionality of blocks label Oct 2, 2017
@tfrommen
Copy link
Member Author

tfrommen commented Oct 2, 2017

I'm also 💯 for simple(r) markup!

Regarding the leading (em) dash, one could easily make this in CSS only (e.g., ::before { content: '—'; }), if one wanted. Talking about themes here, of course, not Gutenberg/Core.

color: $dark-gray-600;
position: relative;
font-weight: 900;
text-transform: uppercase;
font-size: 13px;
}

footer p {
cite p {
Copy link
Member

Choose a reason for hiding this comment

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

cite can only contain phrasing content, so this doesn't look right.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, you're right. However, I just adapted the CSS rules that were there. Currently, a paragraph within a <cite> element is not used anywhere.

Should I just remove that rule then, @iseulde?

Copy link
Member

Choose a reason for hiding this comment

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

I don't know, would need to look into it. Does the cite element still contain paragraphs? Not sure why that rule was there.

Copy link
Member

Choose a reason for hiding this comment

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

Looks like you can just remove it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Like I said, no it does not. I am talking about all the fixtures here. Also, the regular quote does not have this rule, only the pullquote. Also also, seeing <Editable tagName="cite" value={ citation } ..., I'm not sure if paragraphs would actually work. I'd say the citation is not designed to contain more than a reference (and thus, of course, not something in multiple paragraphs).

Copy link
Member

Choose a reason for hiding this comment

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

Right, if it were it would be <Editable multiline="true" />.

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed.

@@ -11,7 +11,7 @@
}

.wp-block-pullquote {
footer .blocks-editable__tinymce[data-is-empty="true"]:before {
Copy link
Member

Choose a reason for hiding this comment

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

Is there ever a scenario where this rule is applied? Looks like this is old and safe to remove.

Copy link
Member Author

Choose a reason for hiding this comment

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

Just tell me (anyone) if I should remove it, or not. 🙂

@mtias
Copy link
Member

mtias commented Oct 4, 2017

One other thing to do here is to make sure we don't invalidate previous quote blocks that were using footer.

@tfrommen
Copy link
Member Author

tfrommen commented Oct 4, 2017

@mtias was that something specifically for me to do, or just a note (for you)? I mean, looking at my changes, which were all occurrences of footer in the context of blockquotes, I don't see what you are referring to. Can you give me any pointers? Thanks.

@tfrommen
Copy link
Member Author

tfrommen commented Oct 4, 2017

Looking again, you might be talking about this:

citation: {
    type: 'array',
    source: children( 'cite' ),
 }

Right?

What would you propose? Just read footer as well, but render it in <cite> now?

// EDIT: Too slow, sorry. 😅

@mtias
Copy link
Member

mtias commented Oct 4, 2017

It can be done with source: children( 'cite, footer' ), which might be alright for this case, but see #2541 for some more general considerations of block versioning :)

@tfrommen
Copy link
Member Author

tfrommen commented Oct 4, 2017

@mtias I chose what you proposed here, and what I personally think the best as well as most intuitive option.

Now, the only thing left here is the potentially obsolete CSS rule, @iseulde mentioned. You have to tell me what to do here, though. 🙂

@@ -2,7 +2,7 @@
margin: 0 0 16px;
box-shadow: inset 0px 0px 0px 0px $light-gray-500;

footer {
cite {
Copy link
Member

Choose a reason for hiding this comment

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

We should also probably keep these rules for a few releases as cite, footer {}.

Copy link
Member Author

Choose a reason for hiding this comment

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

@mtias makes sense—for a while. I just did that.

@tfrommen
Copy link
Member Author

What's in the way for this to get merged? Anything I can do?

What's with that one CSS rule, @iseulde?

@mtias
Copy link
Member

mtias commented Oct 23, 2017

Let's target this one for 1.6. We need to figure out how to prevent invalidating older blocks in the validation process.

@tfrommen
Copy link
Member Author

@mtias isn't that what I did in 0e66d37? We allow both cite and the legacy footer version, but render it as cite.

@mtias
Copy link
Member

mtias commented Oct 24, 2017

@tfrommen that allows both to be valid attributes, but there's also a validation step that compares raw and serialized outputs, and those would differ, so the block would show up as invalid to the user.

You can try it quickly by creating a quote block, clicking on the ellipsis menu for the block, choosing "edit as HTML", and changing the footer with a cite element. You should see this after the change:

image

@danielbachhuber
Copy link
Member

Handled in #3665, per #2647 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Blocks Overall functionality of blocks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants