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

Add Code → Preformatted block transform #9326

Merged
merged 1 commit into from
Aug 30, 2018
Merged

Add Code → Preformatted block transform #9326

merged 1 commit into from
Aug 30, 2018

Conversation

ZebulanStanphill
Copy link
Member

@ZebulanStanphill ZebulanStanphill commented Aug 24, 2018

Description

Adds the ability to transform Code blocks into Preformatted blocks. Due to the similar behavior of the two blocks, the transformation is completely lossless.

How has this been tested?

I made a packaged version of this branch, installed it on my website, created a Code block, added some content and a custom CSS class, and converted it to a Preformatted block successfully, with the content and CSS class being preserved.

Additional notes

I would have also added a Preformatted → Code transformation as well, but since the Code block lacks support for inline HTML elements (it does not use RichText), it would be a lossy conversion.

I made an issue about this: #6672. I would have fixed it in this PR and added the aforementioned Preformatted → Code transformation, but fixing it would require updating the Code block to use a RichText field.

At first, this sounds like no problem. However, RichText fields apparently have issues when they are inline elements. See #8785, #9180, and #9195. The content of the Code block is stored inside a <code> element nested in a <pre> element. The <code> element is an inline element, which means that issues like #9180 could occur if the Code block was updated to use RichText.

You might think that the solution to this issue is simply removing the <code> tags from the Code block, but doing so would remove the semantics that separate the Code block from the Preformatted block, making the former redundant. So unless some solution to the inline RichText issues is found, the Code block is pretty much stuck being unable to support inline elements, unfortunately. 😞

Copy link
Member

@tofumatt tofumatt left a comment

Choose a reason for hiding this comment

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

It's a bit of a bummer to allow the conversion to a Preformatted block but then not back to a Code block.

I'd imagine we want to avoid one-way conversions. Are there other instances of one-way conversions like this? If so I'd say this is fine to merge, but if not I don't think we should implement this.

@ZebulanStanphill
Copy link
Member Author

ZebulanStanphill commented Aug 24, 2018

Are there other instances of one-way conversions like this?

@tofumatt I thought there was, but apparently not. There are lossy conversions, though. You can transform a Cover Image into a Heading, but you lose the background image. I guess that would make it okay to add a Preformatted → Code transformation. Do you think I should do that?

@0aveRyan
Copy link
Contributor

Example of a partial one-way transform:
Single Paragraph gets converted to List (each line === single bullet), however converting back to Paragraph inserts each bullet as a separate paragraph block.
#9162

@tofumatt
Copy link
Member

So the conversion back from Preformatted block to Code block would lose what content exactly? Just any inline HTML? That seems fine to lose on the conversion 🤷‍♂️

@ZebulanStanphill
Copy link
Member Author

@tofumatt Yeah, I guess it is alright. I just had this hypothetical situation in my head where someone had used a bunch of semantic markup in their code snippet (e.g. highlighting a specific line of code using a <strong> element or making function names use <a> elements that link to their documentation), but forgot they were using the Preformatted block instead of the Code block.

But I guess that since all the Code block really does is add <code> tags to the start and end of the content, there is not much reason to use it. Other than theme styling, you could accomplish the same thing in the Preformatted block by wrapping the content with your own <code> element.

But then that makes me wonder if the Code block is even that necessary. If it can't be used for the same kind of advanced situations as the Preformatted block (but involving code), then what is the point? Theme styling and the <code> element being part of the default markup is the only that separates the Code block from the Preformatted block.

I really wish the RichText thing was not an issue. As it is, the Preformatted block is much more flexible than the Code block, which makes the latter feel somewhat redundant. If it were not for the possibility of the RichText thing being fixed, I would say that the Code block might as well be removed, since you can just add a wrapping <code> element to the Preformatted block anyway. If it were not for the <code> element, the Code block might as well just be a style variation of the Preformatted block.

But I guess until either the RichText issue is fixed, some workaround for inline HTML elements is found for the Code block, or the Code block is just removed entirely, it is fine to have a lossy conversion from the Preformatted block to the Code block. I will go ahead and add that to this PR.

@tofumatt tofumatt requested review from tofumatt and removed request for tofumatt August 28, 2018 00:47
@gziolo
Copy link
Member

gziolo commented Aug 28, 2018

We had a similar discussion several times already. We add only transformations which can be reversed without loosing any content. In this case you would loose all html tags so you can’t convert it back to Code block in thar sense.

See also a related PR which didn’t land in master: #5585.

@ZebulanStanphill
Copy link
Member Author

@gziolo So why is Cover Image → Heading (and vice-versa) a valid transform? That is definitely a lossy conversion, and oddly enough, the Cover Image does not even use a heading element, so the transform would make more sense with a Paragraph block. Should the Cover Image → Heading (and vice-versa) transforms be removed?

@gziolo
Copy link
Member

gziolo commented Aug 28, 2018

The conclusion in the PR I linked was that it isn’t necessary a valid transform 😃

@tofumatt
Copy link
Member

Do we have that in a doc anywhere? That’s a good rule, I’m just not sure I’ve run across it reading through docs in the past.

Can this be merged as-is then, with just the one-way transform? Or are you saying we shouldn’t add more one-way transforms either? That seems inconsistent as some exist and the transform here does seem useful.

Copy link
Member

@tofumatt tofumatt left a comment

Choose a reason for hiding this comment

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

Well, this transform works and I don't see why we shouldn't allow it, so I'm going to say this is fine.

It seems like a good candidate for an E2E test, I'll try to cook one up before bed. 😄

@tofumatt tofumatt merged commit 0bfa210 into WordPress:master Aug 30, 2018
tofumatt added a commit that referenced this pull request Aug 30, 2018
@mtias mtias added the [Feature] Block Transforms Block transforms from one block to another label Aug 30, 2018
@ZebulanStanphill ZebulanStanphill deleted the update/add-code-to-preformatted-transform branch September 4, 2018 05:07
@mtias mtias added this to the 3.7 milestone Sep 6, 2018
tofumatt added a commit that referenced this pull request Sep 7, 2018
gziolo pushed a commit that referenced this pull request Sep 25, 2018
@sarahmonster sarahmonster mentioned this pull request Nov 8, 2018
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Block Transforms Block transforms from one block to another
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants