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

Apply CustomHtmlEditorFieldToolbar extension to correct class #46

Closed

Conversation

michalkleiner
Copy link

@michalkleiner michalkleiner commented Aug 23, 2018

Address #45

@robbieaverill
Copy link
Contributor

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

Copy link
Contributor

@robbieaverill robbieaverill left a comment

Choose a reason for hiding this comment

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

Hold til we've tested the javascript this reintroduces

@robbieaverill robbieaverill self-assigned this Aug 26, 2018
@robbieaverill
Copy link
Contributor

robbieaverill commented Aug 26, 2018

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. -> Reintroduce feature to validate that CMS users have entered alt tags for images cwp#149

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

@michalkleiner
Copy link
Author

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
Copy link
Contributor

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
Copy link
Contributor

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.

@michalkleiner michalkleiner deleted the pr/fix-extended-class branch April 4, 2019 21:03
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