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 quick code option to settings and shortcode parameters #126

Merged
merged 4 commits into from
Apr 8, 2020

Conversation

renatho
Copy link
Contributor

@renatho renatho commented Mar 13, 2020

Description

This PR introduces a new setting option and shortcode to toggle the quickcode (currently always enabled).

The quickcode is responsible for the feature that allows the user to enter in edit mode when double-clicking on the code. This PR keeps it turned on by default, but allowing to disable, what can be useful for some websites.

Screenshots

Screen Shot 2020-03-13 at 18 18 32

The behaviour after the double click (current behaviour): Remove the formatting and allow the user to edit the code.

It allows to toggle the feature that allows the user to enter in
edit mode when double clicking.
Copy link
Contributor

@alexsanford alexsanford left a comment

Choose a reason for hiding this comment

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

This looks good and works well!

One inconsistency is that we do not have this option for the Block in the Block Editor. We can turn it on/off globally, and within individual shortcodes. Perhaps we can add a Block setting as well? (Can be in a new PR)

@@ -1392,6 +1399,7 @@ function settings_page() { ?>
<label for="syntaxhighlighter-light"><input name="syntaxhighlighter_settings[light]" type="checkbox" id="syntaxhighlighter-light" value="1" <?php checked( $this->settings['light'], 1 ); ?> /> <?php _e( 'Use the light display mode, best for single lines of code', 'syntaxhighlighter' ); ?></label><br />
<label for="syntaxhighlighter-smarttabs"><input name="syntaxhighlighter_settings[smarttabs]" type="checkbox" id="syntaxhighlighter-smarttabs" value="1" <?php checked( $this->settings['smarttabs'], 1 ); ?> /> <?php _e( 'Use smart tabs allowing tabs being used for alignment', 'syntaxhighlighter' ); ?></label><br />
<label for="syntaxhighlighter-wraplines"><input name="syntaxhighlighter_settings[wraplines]" type="checkbox" id="syntaxhighlighter-wraplines" value="1" <?php checked( $this->settings['wraplines'], 1 ); ?> /> <?php _e( 'Wrap long lines (v2.x only, disabling this will make a scrollbar show instead)', 'syntaxhighlighter' ); ?></label><br />
<label for="syntaxhighlighter-quickcode"><input name="syntaxhighlighter_settings[quickcode]" type="checkbox" id="syntaxhighlighter-quickcode" value="1" <?php checked( $this->settings['quickcode'], 1 ); ?> /> <?php _e( 'Enable edit mode on double click', 'syntaxhighlighter' ); ?></label><br />
Copy link
Contributor

Choose a reason for hiding this comment

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

We should use esc_html_e( ... ) here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! I took the opportunity to fix all occurrences.

c3debae

Maybe I could do it in another PR, but as I was changing some of these texts, I think it also makes sense to be in this PR. Makes sense to you? If you think not, I can split ;)

@@ -1516,6 +1524,7 @@ function settings_page() { ?>
<li><?php printf( _x( '%1$s (v3 only) &#8212; Sets some text to show up before the code. Very useful when combined with the %2$s parameter.', 'title parameter', 'syntaxhighlighter' ), '<code>title</code>', '<code>collapse</code>' ); ?></li>
<li><?php printf( _x( '%s &#8212; Toggle the toolbar (buttons in v2, the about question mark in v3)', 'toolbar parameter', 'syntaxhighlighter' ), '<code>toolbar</code>' ); ?></li>
<li><?php printf( _x( '%s (v2 only) &#8212; Toggle line wrapping.', 'wraplines parameter', 'syntaxhighlighter'), '<code>wraplines</code>' ); ?></li>
<li><?php printf( _x( '%s &#8212; Enable edit mode on double click.', 'quickcode parameter', 'syntaxhighlighter' ), '<code>quickcode</code>' ); ?></li>
Copy link
Contributor

Choose a reason for hiding this comment

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

We should use esc_html_x( ... ) here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! I took the opportunity to fix all occurrences.

c3debae

Maybe I could do it in another PR, but as I was changing some of these texts, I think it also makes sense to be in this PR. Makes sense to you? If you think not, I can split ;)

@alexsanford alexsanford added this to the 3.5.3 milestone Apr 3, 2020
@renatho renatho force-pushed the add/quick-code-option branch from c3debae to 0afc0e6 Compare April 4, 2020 19:31
@renatho
Copy link
Contributor Author

renatho commented Apr 4, 2020

One inconsistency is that we do not have this option for the Block in the Block Editor. We can turn it on/off globally, and within individual shortcodes. Perhaps we can add a Block setting as well? (Can be in a new PR)

@alexsanford Makes sense!

I added it here 53143c6

Thank you for your code review so far. =)

@renatho renatho requested a review from alexsanford April 6, 2020 14:19
@alexsanford
Copy link
Contributor

@renatho The feature on the block works well! I wonder, though, if we should give a better explanation. The Setting, for example, says "Enable edit mode on double click (v3.x only)", whereas the block setting says "Quick code". I don't think that will be clear to people.

So two things:

  • Could we either change the text of the setting label, or add helper text to clarify what it does?

  • Since the Quick Code option is only available for v3.x, I wonder if it would make sense to only make the block setting available if we are using v3.x of the library?

@renatho
Copy link
Contributor Author

renatho commented Apr 7, 2020

Hey @alexsanford ! Thank you for your re-review! 😅

Could we either change the text of the setting label, or add helper text to clarify what it does?

Makes totally sense! I updated the block editor to match the same setting label: 9e736a0

Since the Quick Code option is only available for v3.x, I wonder if it would make sense to only make the block setting available if we are using v3.x of the library?

Actually, it should be working like that because of these lines:

I tested and it seems to be working. Could you please confirm if it is not working?

The test that I did:

  1. Go to /wp-admin/options-general.php?page=syntaxhighlighter.
  2. Update the Highlighter Version to 2 / 3.
  3. Test adding the block to a post and check the options in the sidebar.

Copy link
Contributor

@alexsanford alexsanford left a comment

Choose a reason for hiding this comment

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

Actually, it should be working like that because of these lines

Ah, yes it is! Sorry about that. I noticed the text in the setting, and didn't even test to see if it was already working. Whoops! 😅

This looks great, and works well! 🎉

@renatho renatho merged commit 3837bbc into master Apr 8, 2020
@alexsanford alexsanford deleted the add/quick-code-option branch April 8, 2020 16:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants