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

Removed unused system settings for "Rich-Text Editor" section #14843

Merged
merged 4 commits into from
Nov 29, 2019
Merged

Removed unused system settings for "Rich-Text Editor" section #14843

merged 4 commits into from
Nov 29, 2019

Conversation

Ruslan-Aleev
Copy link
Collaborator

What does it do?

Removed unused system settings for "Rich-Text Editor" section.

These settings are found only in the "System Settings", do not participate in the remaining code:

  • editor_css_path
  • editor_css_selectors

In the future, I will check the settings in other sections.

Related issue(s)/PR(s)

#14539 (comment)

@Mark-H
Copy link
Collaborator

Mark-H commented Nov 26, 2019

I think the old tinymce package may use this?

That's obviously a poor reason to keep it in core, but if that's the case it's a good idea to make sure the package is updated to use a different setting first.

@Ruslan-Aleev
Copy link
Collaborator Author

I think the old tinymce package may use this?

@Mark-H I searched for this setting on github, there were no direct matches, only in MODX, as it seems to me.

@Ruslan-Aleev
Copy link
Collaborator Author

The old version of tinyMCE will not work with MODX 3, in any case. After all, the version of the editor after the release of MODX 3 will have to be updated for compatibility with MODX 3, and it would be disadvantageous to update the old version tinyMCE in any case.

@JoshuaLuckers
Copy link
Contributor

I think the old tinymce package may use this?

That's obviously a poor reason to keep it in core, but if that's the case it's a good idea to make sure the package is updated to use a different setting first.

Seems like something that should be documented in the docs (Upgrading MODX).

@Mark-H
Copy link
Collaborator

Mark-H commented Nov 29, 2019

I've opened an issue for TinyMCE and for the 3.0 upgrade documentation, but am wondering if perhaps we should make a special exception for these settings and don't actively remove them upon upgrade?

While that would leave an orphaned setting, that setting could potentially contain information that's still relevant for people's site.

On the other hand... that's a really old extra and I don't see any documentation explaining users how to use it, so we could also just go ahead with this and let Tiny figure it out. I'm a little conflicted, so opinions are welcome.

@JoshuaLuckers
Copy link
Contributor

Maybe remove the settings only when the value equals the default install value?

Copy link
Collaborator

@Mark-H Mark-H left a comment

Choose a reason for hiding this comment

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

I've decided it can't hurt to force tinymce to update for the first time in a few years while cleaning up the core.

Mark-H added a commit that referenced this pull request Nov 29, 2019
Merge remote-tracking branch 'upstream/pr/14843' into 3.x
@Mark-H Mark-H merged commit 947194f into modxcms:3.x Nov 29, 2019
@donShakespeare
Copy link

If no editor is the default editor for MODX there might be no planetary reason for keeping a specific strictly-related editor setting lying around in the MODX core.

Wise decision!

@Ruslan-Aleev Ruslan-Aleev deleted the 3.x-fix-14539-2 branch June 18, 2020 08:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants