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

Fix adding classname when using SyntaxHighlighter block #138

Merged
merged 2 commits into from
Apr 24, 2020

Conversation

renatho
Copy link
Contributor

@renatho renatho commented Apr 13, 2020

Fixes #123 and fixes #130

Changes proposed in this Pull Request:

  • Fix adding classname when using Syntax Highlighter block

Testing instructions:

  • Create a new Syntax Highlighter block in a post or page.
  • In the sidebar, AdvancedAdditional CSS class(es) add a class.
  • Open the page on the frontend and check if the class was added.

@renatho renatho self-assigned this Apr 13, 2020
@renatho renatho modified the milestones: v3.5.4, v3.5.5 Apr 13, 2020
Copy link
Contributor

@gikaragia gikaragia left a comment

Choose a reason for hiding this comment

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

So after testing this a bit I have found out that only the first class is included in the frontend.

The HTML that the plugin generates it is correct (it includes all classes) however the SyntaxHighlighter JS library modifies the HTML and it only adds the first class. It's supposed to support multiple classes however I wasn't able to make it work and reading its source code didn't help.

I think that we should merge this fix but leave issue #130 open, lower its priority and repurpose it to mention that multiple classes are not supported. Not sure if a fix is possible without modifying the dependency library though.

@renatho
Copy link
Contributor Author

renatho commented Apr 23, 2020

So after testing this a bit I have found out that only the first class is included in the frontend.

Good catch!

I realized here that he uses single quotes in the class-name attribute. So I tested and it worked with multiple classes \o/

Fixed here: 53786a3

The name of the method where I fixed it is shortcode_callback, but the render_block also calls this method. Alex leave this comment in the render_block method:

Renders the content of the Gutenberg block on the front end using the shortcode callback. This ensures one source of truth and allows for forward compatibility.

@renatho renatho requested a review from gikaragia April 23, 2020 21:55
@gikaragia
Copy link
Contributor

Nice! Didn't thought of using quotes :)

@renatho renatho merged commit 4e7a6b7 into master Apr 24, 2020
@renatho renatho deleted the fix/classname-through-block branch April 24, 2020 13:46
@renatho renatho changed the title Add parse classname attribute to syntaxhighlighter block Fix adding classname when using SyntaxHighlighter block Jun 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants