Apply CustomHtmlEditorFieldToolbar extension to correct class

michalkleiner commented 6 years ago

Address #45

robbieaverill commented 6 years ago

Thanks - I'd like to test the Javascript that this change will reintroduce before we merge

robbieaverill commented 6 years ago

Alright, here's the summary of what's going on:

The config namespace was wrong and this PR fixes it
The extension adds some custom javascript and translations on the updateMediaForm hook
That extension hook doesn't exist in core any more, so the code is not being executed anyway

It looks like the associated javascript file is trying to ensure that users enter alt tags in their images in the CMS. This is great, but isn't working on SilverStripe 4 and will probably need to be rewritten.

My suggestions:

Remove the extension config for the CustomHtmlEditorFieldToolbar class entirely (patch change -> 2.1)
Deprecate the CustomHtmlEditorFieldToolbar class (minor change -> 2.2)
Delete CustomHtmlEditorFieldToolbar.js: justifying this as a patch change because it's not currently in use by default, and wouldn't work with SS4 assets even if it were (patch change -> 2.1)
Raise new issue in cwp/cwp to reintroduce this feature. It should be logged in that repository since this repository is more for core platform support on CWP rather than CMS features. -> https://github.com/silverstripe/cwp/issues/149

@michalkleiner would you be keen to make the changes I've suggested? I'll raise the CWP issue now.

michalkleiner commented 6 years ago

Would it mean two PRs against different branches? Should this one be closed and a new one opened or do I rename it and do the patch changes here, minor changes in a new one.. So many options. What's your preference @robbieaverill ?

robbieaverill commented 6 years ago

Up to you @michalkleiner - there'd be at least one PR for 2.1 and one for 2.2. Since this PR targets master you may as well close and make new ones, but equally you could retarget this PR at 2.1 and re-use it. Up to you, I have no strong preference =)

robbieaverill commented 5 years ago

We've got a ticket coming up to rewrite this functionality so it works in SS4. Thanks for the PR, but we don't want to re-enable this class.

silverstripe / cwp-core

Apply CustomHtmlEditorFieldToolbar extension to correct class #46