albertgasset / dokuwiki-plugin-codemirror

This project is now hosted on GitLab https://gitlab.com/albertgasset/dokuwiki-plugin-codemirror
GNU General Public License v2.0
17 stars 15 forks source link

Adding 'autoheight' option and cleaning up code for usenativescroll #69

Closed bkidwell closed 7 years ago

bkidwell commented 7 years ago

My 'usenativescroll' option patch from the other day was a little rough and I've fixed that.

I'm also adding a new "autoheight" option that I need: When autoheight is turned on and CodeMirror editor is active, the "larger / smaller" buttons are hidden, and the CodeMirror work area is sized to fit the height of its content.

Sorry this got jumbled into one commit; I ended up working on both problems at the same time and couldn't easily separate the work.

bkidwell commented 7 years ago

I should explain why I want to add an "autoheight" option...

I understand that under normal circumstances, it might be problematic to have a very long CodeMirror widget and have the formatting toolbar scroll off the top of the window.

I'm working on a plugin called "editclean" which does the following on the edit screen:

In order to finish this off, I wanted an option in CodeMirror to remove the inner scrollbar for the page content edit widget.

Currently my "editclean" plugin only works with "Bootstrap3" theme. I will publish it soon -- hopefully after making it also compatible with the DokuWiki default theme.

albertgasset commented 7 years ago

Sorry, I understand what you want to achieve but there are several thinks I don't like about your solution:

  1. The auto-height setting make senses only as a global setting, I don't thinks it's useful in the menu of options. It's not something you would like to change while you're editing. The same for the native scrollbar setting.
  2. You should not modify initCodeMirror and destroyCodeMirror functions. Use the callback function of each setting for applying/unapplying the effects of the settings.
  3. Adding/removing a <style> tag looks ugly. You can use jQuery to set the height to auto. When the setting is switched of you can recover the original height from textarea.css('height').
  4. To show/hide images, you can use a shorter way with CSS selectors: jQuery('#size__ctl img[src$="/larger.gif"], #size__ctl img[src$="/smaller.gif"]').hide().
bkidwell commented 7 years ago

Thanks for the detailed feedback. I'll get back to you in a few days or a week.

bkidwell commented 7 years ago

Again @albertgasset thanks for your detailed feedback. Please review my reworked patch and see if you'd like to merge it now into your master branch.

  1. 'autoheight' and 'usenativescroll' should be a global setting and not available in edit screen UI.

Agreed; I removed it from the edit screen's options menu. I had to add a 'noCookie' attribute on these two settings and add some code to getSetting() and setSetting() to make sure not to use the the DokuWiki PREFS cookie. The cookie value if present overrides the sysadmin's default choice if they get written and then the sysadmin changes the default.

  1. Don't modify initCodeMirror() and destroyCodeMirror().

I removed everything I could remove from these two functions, but there's still one line of code to set the viewportMargin CodeMirror option; setting this after CodeMirror is initialized doesn't seem to work.

  1. Don't use add and remove HTML 'style' elements.

Fixed.

  1. Suggestion of better jQuery code to show/hide the "larger" and "smaller" buttons.

Implemented.

bkidwell commented 7 years ago

I realized I hadn't tested it right and I didn't need to set the 'native scroll bar' option in initCodeMirror().

Sorry for the flurry of commits right after my "please check it out again" comment not long ago.

My revised response follows...

  1. 'autoheight' and 'usenativescroll' should be a global setting and not available in edit screen UI.

Agreed; I removed it from the edit screen's options menu. I had to add a 'noCookie' attribute on these two settings and add some code to getSetting() and setSetting() to make sure not to use the the DokuWiki PREFS cookie. The cookie value if present overrides the sysadmin's default choice if they get written and then the sysadmin changes the default.

  1. Don't modify initCodeMirror() and destroyCodeMirror().

Fixed.

  1. Don't use add and remove HTML 'style' elements.

Fixed.

  1. Suggestion of better jQuery code to show/hide the "larger" and "smaller" buttons.

Implemented.

albertgasset commented 7 years ago

This looks much better now, thanks!

In the first review I said two contradictory things, I'm sorry. In point 1 I told you not to use menu settings, and in point 2 I told you to use the callback function for menu settings... For global settings it's okay to modify initCodeMirror(), like you did for the native scroll setting.

Can you move the initialization code back to initCodeMirror() and destroyCodeMirror()? This way you don't need the noCookie modifications in getConfig() and setConfig(), and you can read the settings directly from JSINFO.plugin_codemirror. Again, sorry for misleading you.

bkidwell commented 7 years ago

No problem at all, @albertgasset. Expect a further revision of this patch by Friday or so. Thanks for the very useful plugin.

bkidwell commented 7 years ago

I dropped the ball on this. I'll get to it this week. Sorry for wandering off.

bkidwell commented 7 years ago

@albertgasset, I finally spent half an hour making the final fixes to this patch that you requested. Please review and/or merge when it's convenient for you.

albertgasset commented 7 years ago

Thanks!