gjbarnard / moodle-filter_synhi

Syntax highlighter
GNU General Public License v3.0
3 stars 2 forks source link

Added enlighterJS enable/disable selector settings #10

Closed cli-ish closed 1 year ago

cli-ish commented 1 year ago

Hi, I have evaluated your points from my closed PR and applied them to this PR. If I have attacked you in any way, I am very sorry that was not my intention.

This PR deals with the following feature: Added two new settings to change the selectors that are passed to enlighterjs. There are still the two selectors already defined by you: synhi code synhi pre plus another selector none which can be used to disable one of the selectors.

grafik

gjb2048 commented 1 year ago

Dear @cli-ish,

You didn't attack me, I just put my head in my hands in despair at the code. Ok, still issues:

  1. Why is this change required?
  2. You still are allowing the 'synhi' tag in the setting options.
  3. You've not read the EnlighterJS API documentation correctly (https://github.com/EnlighterJS/documentation/blob/master/development/Methods_and_API.md) where 'NONE' means 'null', thus in the third option of the setting you're telling EnlighterJS.init to look for the 'none' selector!
  4. Possibly need PHPUnit / Behat tests to prove that this works.

G

cli-ish commented 1 year ago

Thank you for your feedback,

  1. This feature is required since the filter.php:filter will apply the enlighterjs to all pre and code tags (synhi will be wrapped around the whole text when a pre/code tag is found https://github.com/gjb2048/moodle-filter_synhi/blob/master/filter.php#L54-L78).

grafik

grafik

  1. I will apply this to the PR
  2. logic will replace none with javascript null (https://github.com/gjb2048/moodle-filter_synhi/pull/10/files#diff-33552c0b997f7b199f66c9bb89baeef2047b889c15ce7afe5501c5af864ec30eR244)
  3. i don't see why this should be required, but if desired i could add such a commit.
gjb2048 commented 1 year ago

Dear @cli-ish,

Thank you, so please can you tell me 'why' you're spending your time on this please? And you're not sure why testing is needed?

G

cli-ish commented 1 year ago

Dear @gjb2048 ,

Thanks for your time, we will keep the changes in our own repository then.