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

Wrap cite with footer in saved markup for quote and pullquote blocks #9804

Closed
wants to merge 1 commit into from
Closed

Wrap cite with footer in saved markup for quote and pullquote blocks #9804

wants to merge 1 commit into from

Conversation

ZebulanStanphill
Copy link
Member

Description

This PR changes the Quote and Pullquote blocks to wrap their <cite>s with <footer>s. The reason for this is that <cite> tags can be used within a quote but not be the citation of a quote, e.g. using a <cite> element to refer to a book or ship. The use of a <footer> improves the semantics to show that the contained <cite> is the citation of the entire <blockquote>, and makes it easier for themes to style inline <cite>s differently from the one in the <footer>.

In the editor, the RichText field for the citation is a <footer> element, where previously it was a <div> due to #8785 and before that it was a <cite>.

How has this been tested?

I have tested to make sure that old Quote and Pullquote blocks are automatically converted properly. Some errors appear in the JavaScript console the first time a post is opened containing the deprecated forms of the blocks, but I am guessing that is due to the deprecated forms not matching the other even older deprecated forms before matching the last one in the list. The content is preserved and automatically converted as expected and no blocks are made invalid. After saving, no errors appear in the JavaScript console.

(I would say that the failed matches of deprecated forms should not be called out in the JavaScript console as errors, but I am not sure of the history of this functionality or how it works, and that is out-of-scope for this PR anyway.)

Additional info

@ZebulanStanphill
Copy link
Member Author

Test failures seem unrelated.

@joyously
Copy link

Shouldn't any cite tags be user supplied? And the citation for the quote is in a cite attribute?

@ZebulanStanphill
Copy link
Member Author

@joyously The cite attribute would be used for a hidden citation, not a visible one. You do not need the former if you have the latter.

I think it is assumed by the Quote block is that the only thing you are putting in the footer is the citation. I would not be opposed to removing the <cite> element (which is in the current form of the Quote block in both the release version and master) if that assumption was wrong, though. But as far as I can tell, the Quote block assumes you are only putting the citation below the quote.

Additionally, most people are going to just put citations in the footer. If they want to get really fancy with what is in the footer, then I guess a Custom HTML block is the best answer for them. As it is, assuming that the only content in the footer is a citation seems fine to me, but if everyone else wants to remove the <cite> tag I won't complain. I just assumed that the Quote block markup should provide the best semantics for the majority of the ways it is going to be used. The way my PR makes it is so that more use-cases are supported than in master, while also automatically providing proper semantics for the average user.

Whatever the case, this is more semantically correct than what is currently in master, and definitely better for styling, since you can target a quote citation without targeting <cite> elements used for things like book, TV show, or game titles.

@joyously
Copy link

It's not true that you don't need the attribute if you have a visible cite tag. The attribute is really for a URL.
I think it would be better if the cite was outside of the blockquote, so that the blockquote formatting does not affect it.
Also, it looks like one place in your PR, you have new code that looks like the old (no footer).

@chrisvanpatten
Copy link
Contributor

chrisvanpatten commented Sep 11, 2018

This feels like a nice iterative improvement over the existing behavior!

A discussion on whether or not to move the citation out of the <blockquote> element is worth having, but in a separate issue. I also think it'd be nice to expose the cite attribute—perhaps as an inspector control?—but that's also better for another issue or PR.

I think it'd be good to have another reviewer look at the console notice but semantically this makes sense to me and I like that it simplifies the CSS a smidge. Great stuff Zeb!

@ZebulanStanphill
Copy link
Member Author

@joyously

It's not true that you don't need the attribute if you have a visible cite tag. The attribute is really for a URL.

Thanks for pointing that out. I was mistaken. I agree with @chrisvanpatten that the cite attribute should be shown in the inspector.

Also, it looks like one place in your PR, you have new code that looks like the old (no footer).

That is intentional, because that is part of a definition of the deprecated form of the block, not the current form.

I think it would be better if the cite was outside of the blockquote, so that the blockquote formatting does not affect it.

I agree with @chrisvanpatten that that might be a good thing to explore in another PR. However, as it is, the current method clearly associates the <cite> with the <blockquote>, and being inside the <footer> provides a way to keep its styling separate from other <cite>s, so I am not really sure if moving the <cite> outside of the <blockquote> is necessary.

@ZebulanStanphill
Copy link
Member Author

Due to recent changes, this PR needed a rebase due to merge conflicts, but so much had changed that it was easier to create a new PR entirely: #10465.

@ZebulanStanphill ZebulanStanphill deleted the update/wrap-cite-with-footer-in-quote-and-pullquote branch October 10, 2018 04:13
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.

3 participants