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

Navigation: formatting the allowed styles #19477

Merged
merged 2 commits into from
Jan 8, 2020

Conversation

retrofox
Copy link
Contributor

@retrofox retrofox commented Jan 7, 2020

Description

This PR handles rightly applying HTML format to menu items, avoiding escape HTML characters for those ones which are allowed.

Fixes #19473

How has this been tested?

You can format the menu items using the toolbar of the navigation menu. So far, it's possible to set up bold, italic, code, underline, strikethrough, span text, and inline-image.

image

In order to test it, just apply some format to the navigation item and confirm that the format s applied as expected in the front-end.

Screenshots

image

Types of changes

Checklist:

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

Copy link
Member

@WunderBart WunderBart left a comment

Choose a reason for hiding this comment

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

Tested by applying a bunch of formatting options - looks good on both front & back-end!

Screenshot 2020-01-07 17 09 57

@retrofox retrofox added [Block] Navigation Affects the Navigation Block [Type] Enhancement A suggestion for improvement. labels Jan 7, 2020
@retrofox retrofox changed the title navigation: formatting the allowed styles Navigation: formatting the allowed styles Jan 7, 2020
Copy link
Contributor

@getdave getdave left a comment

Choose a reason for hiding this comment

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

Nice work.

I tested this and all the formats appeared to apply correctly in Editor and on front end.

Frontend

Screenshot 2020-01-08 at 09 42 53

Editor

Screenshot 2020-01-08 at 09 42 41

@retrofox
Copy link
Contributor Author

retrofox commented Jan 8, 2020

Thanks for reviewing, folks!

@draganescu draganescu merged commit 14a458a into master Jan 8, 2020
@draganescu draganescu deleted the update/navigation-fix-format branch January 8, 2020 12:16
@draganescu
Copy link
Contributor

@retrofox be careful with that planet destroyer!

@retrofox
Copy link
Contributor Author

retrofox commented Jan 8, 2020

@retrofox be careful with that planet destroyer!

No kidding, he's so cute as destructive! 😱

@maheshwaghmare
Copy link
Contributor

Nice work!

I was also thinking to create a PR but @retrofox did nice work.


Below are my findings.

Backend: https://share.getcloudapp.com/xQu0zkwJ

Before Code:

$html .= esc_html( $block['attrs']['label'] );

Before Front End : https://share.getcloudapp.com/ApuOxoe5

After Code:

$html .= wp_kses_post( $block['attrs']['label'] );

After Front End: https://share.getcloudapp.com/p9u5X4Z1

@retrofox
Copy link
Contributor Author

retrofox commented Jan 8, 2020

Thanks, @maheshwaghmare for your comment.

After Code:

$html .= wp_kses_post( $block['attrs']['label'] );

I did a quick research about how to tackle this issue and found wp_kses_post() as well. The thing is if I'm not wrong wp_kses() fits better on this solution since it allows us to define the allowed tags and, since they are just a few, the performance will be better than using the wp_kses_post().

@obenland
Copy link
Member

obenland commented Jan 8, 2020

I wouldn't be too concerned about performance with strings as short as we can expect in a label

@retrofox
Copy link
Contributor Author

retrofox commented Jan 8, 2020

I wouldn't be too concerned about performance with strings as short as we can expect in a label

So we could consider switching to wp_kses_post() since if we add a new HTML tag then we will need to update the list of wp_kses().

@obenland
Copy link
Member

obenland commented Jan 8, 2020

oh, my bad, yes absolutely, I think I'd prefer wp_kses_post() here as well

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Navigation Affects the Navigation Block [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Navigation Block outputs raw HTML on Frontend Site
7 participants