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

[Task] Update RichText usage to avoid inline elements #8785

Merged
merged 1 commit into from
Sep 10, 2018
Merged

[Task] Update RichText usage to avoid inline elements #8785

merged 1 commit into from
Sep 10, 2018

Conversation

nfmohit
Copy link
Member

@nfmohit nfmohit commented Aug 9, 2018

Description

This PR closes #8773 which reports the incompatibility of inline RichText elements and TinyMCE.

How has this been tested?

This PR has been tested by going through the following steps:

  1. Started a new post using the Gutenberg editor.
  2. Added a quote/pull-quote block with some text.
  3. Made sure everything is functional and the styling is unchanged even though inline tags (cite) are replaced with block-level elements (div).

This was tested in WP 4.9.8, Gutenberg 3.4.0, Apache server with PHP 7.2.0 and MySQL 5.6.34. According to initial tests, the code doesn’t seem to affect any other areas.

Screenshots

pull-8773-min

Types of changes

This PR omits the tagName properties in the citations for the quote and pull-quote blocks so that they convert to using div tags instead of cite. It also updates styling to make sure the blocks look unchanged.

Checklist:

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

@@ -86,6 +85,7 @@ export const settings = {
citation: nextCitation,
} )
}
className={ `citation` }
Copy link
Member

Choose a reason for hiding this comment

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

  • This class name is not adequately unique so as to avoid conflicts. Our CSS Naming Guidelines don't necessarily apply to blocks, but there are some general conventions here e.g. wp-block-pullquote__citation
  • In JSX, you can assign a string prop value via the simpler className="wp-block-pullquote__citation" syntax

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, should've been more careful. Addressed in this commit. Thank you @aduth ❤️

@@ -4,9 +4,11 @@
color: $dark-gray-600;

cite,
footer {
footer,
.citation {
Copy link
Member

Choose a reason for hiding this comment

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

These styles are enqueued on the front-end, but we'll never have .citation on the front-end display of this block. Conversely, there won't ever be a cite in the editor to match.

I think we may want this in editor.scss instead? I'm not entirely sure what our practice here is with theme.scss and editor-specific styles (cc @jasmussen ).

Copy link
Member Author

Choose a reason for hiding this comment

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

Here's where I got confused as well. Putting editor specific styles in editor.scss makes more sense to me. But I noticed no styles from theme.scss were being rendered in the frontend, but only in the back-end.

I haven't changed the location of the CSS yet but can do if you prefer. But would love to hear about the practice with theme.scss and editor-specific styles as well. Thank you ❤️

@aduth
Copy link
Member

aduth commented Aug 13, 2018

I think merging in master went poorly here. You might have more luck with git rebase origin/master.

Feel free to ping me on Slack if you need assistance.

@nfmohit
Copy link
Member Author

nfmohit commented Aug 16, 2018

Not sure what was going on with rebase. Tried different workarounds, but ended up with resetting the branch and doing the changes again. Looking forward to another review. Thank you @aduth ❤️

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.

Apologies for the delay in revisiting this for review. It works great! Thanks for the patience and the work here 👍

@aduth aduth added the [Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable label Sep 10, 2018
@aduth aduth added this to the 3.9 milestone Sep 10, 2018
@aduth aduth merged commit 47cd11f into WordPress:master Sep 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Blocks: Update RichText usage to avoid inline elements
2 participants