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

AN-54 Addition of basic HTML support #270

Merged
merged 9 commits into from Jan 11, 2017
Merged

Conversation

kevinfodness
Copy link
Member

  • Adds HTML support option in the Settings area.
  • Adds HTML support to Body elements if enabled in Settings.
  • Adds a new Parser class to handle the split between Markdown and HTML.
  • Refactors the Markdown class to conform to WordPress and PHP best practices.
  • Expands definition of Body elements to include <pre> tags.
  • Adds settings for monospaced font display.
  • Adds global styles for monospaced fonts if HTML is enabled.
  • Adds tests for HTML in Body elements.

Fixes #222.
Fixes #232.

Copy link

@timatron timatron left a comment

Choose a reason for hiding this comment

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

🌊 Just read through my comments. Just things that popped out at me.

@@ -288,6 +288,28 @@ function __construct( $page ) {
'label' => __( 'Pull quote transformation', 'apple-news' ),
'type' => array( 'none', 'uppercase' ),
),
'monospaced_font' => array(
'label' => '',

Choose a reason for hiding this comment

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

no label?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, that's the convention for the font dropdown on all fields that have fonts. It's a good point, though. I'll add it in.

'tracking' => intval( $this->get_setting( 'monospaced_tracking' ) ) / 100,
'lineHeight' => intval( $this->get_setting( 'monospaced_line_height' ) ),
'textColor' => $this->get_setting( 'monospaced_color' ),
'paragraphSpacingBefore' => 18,

Choose a reason for hiding this comment

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

where does this come from? should it be a setting? (same with the line below)

Copy link
Member Author

Choose a reason for hiding this comment

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

This may be moved to a setting at some point in the future. We're discussing adding additional customization options.

@@ -0,0 +1,123 @@
<?php

Choose a reason for hiding this comment

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

Please check tabbing between the various php files, some seem to have spaces, some tabs, might just be how it looks in github, /shrug

Copy link
Member Author

Choose a reason for hiding this comment

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

There is an open issue for moving everything to WP standards, so this is on the roadmap. I'm trying to refactor as I go, but if I'm only applying minor changes to a file, I won't refactor the entire thing.

case 'em':
return $this->_parse_node_emphasis( $node );
case 'br':
return ' ' . "\n";

Choose a reason for hiding this comment

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

space necessary b4 \n?

Copy link
Member Author

Choose a reason for hiding this comment

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

This was in the code previously (I just refactored it) so I believe it is necessary, but can check.

private function parse_text_node( $node ) {
return str_replace( '!', '\\!', $node->nodeValue );
private function _parse_node_emphasis( $node ) {
return '_' . $this->parse_nodes( $node->childNodes ) . '_';

Choose a reason for hiding this comment

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

there must be something magical about the underscores I dont undestand 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

It's converting to markdown, so <em>text</em> becomes _text_.


return sprintf(
'%s %s' . "\n",
str_repeat( '#', $matches[1] ),

Choose a reason for hiding this comment

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

check that it matches b4 you use 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.

This point in the code is never reached unless an explicit match was already found, so $matches[1] will never be empty. However, your comment made me think about the fact that a preg_match is overkill for what we're doing here, so I can convert to a substr.


return "- " . $this->parse_nodes( $node->childNodes );
private function _parse_node_text( $node ) {
return str_replace( '!', '\\!', $node->nodeValue );

Choose a reason for hiding this comment

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

double slashes, what is this foreign language i dont understand?

Copy link
Member Author

Choose a reason for hiding this comment

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

Markdown.

* @access public
*/
public function __construct( $format = 'markdown' ) {
$this->format = ( $format === 'html' ) ? 'html' : 'markdown';

Choose a reason for hiding this comment

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

yoda plz

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch.

libxml_use_internal_errors( false );

// Find the first-level nodes of the body tag.
$nodes = $dom->getElementsByTagName( 'body' )->item( 0 )->childNodes;

Choose a reason for hiding this comment

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

dumb question, but can you assume 'body' exists?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, in a DOMDocument context, it will always exist.

@@ -92,7 +92,7 @@ public function write_json( $content ) {
// JSON should be decoded before being stored.
// Otherwise, stripslashes_deep could potentially remove valid characters
// such as newlines (\n).s
$decoded_json = json_decode( sanitize_text_field( $json ) );
$decoded_json = json_decode( $json );

Choose a reason for hiding this comment

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

dunno why this is changing, but just make sure you wanna do this:)

Copy link
Member Author

Choose a reason for hiding this comment

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

It's changing because we're adding support for HTML, so if you run sanitize_text_field on JSON that contains HTML it hoses it. The HTML output is being run through wp_kses with a strict set of tag and attribute restrictions.

@kevinfodness kevinfodness merged commit aac3157 into master Jan 11, 2017
@kevinfodness kevinfodness deleted the AN-54-html-support branch January 11, 2017 15:23
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.

None yet

2 participants