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

[4.0] TinyMCE skins using template overrides #29259

Closed
wants to merge 8 commits into from

Conversation

dgrammatiko
Copy link
Contributor

@dgrammatiko dgrammatiko commented May 29, 2020

Pull Request for Issue # .
A redo of #14510

Summary of Changes

This a PR that fixes an old bad implementation of the theming for tinyMCE (I'm the one that did that)
In J3 to use a custom skin you had to upload it in the media/editors/tinymce/skins
That's utterly wrong. The skin is part of templating so the right path is templates/mytemplate/
Also the only ways to reach media/editors/tinymce/skins are either ftp or ssh. By using the template we can use the editor in the template styles at the end the skin is just plain css so it belongs to the rest of our current css...

Testing Instructions

Go to node_modules/tinyMCE/skins and copy the folder oxide_dark to administrator/templates/atum/css/vendor/tinymce/skins/ui. Rename it or leave it as is.
Check it by editing an article.

Expected result

Screenshot 2020-05-29 at 14 56 15

Actual result

Documentation Changes Required

Mild B/C break as we're removing a very idiomatic implementation of tinyMCE theming

This is not conflicting with #29257 That PR is adjusting the inner part of the editor

@brianteeman brianteeman mentioned this pull request May 29, 2020
@SharkyKZ
Copy link
Contributor

This unnecessarily removes the ability to select skin per set.

@dgrammatiko
Copy link
Contributor Author

dgrammatiko commented May 29, 2020

This unnecessarily removes the ability to select skin per set.

Yes this is right! the skin should be in sync with the template, it's a css, no need for different sets. Unless you like circus looking sites

Also it's not quite accurate you can still do whatever you want by overriding the tinyMCE options, a known way for getting other options your way...

@joomla-cms-bot joomla-cms-bot added the Language Change This is for Translators label May 29, 2020
@Quy
Copy link
Contributor

Quy commented May 29, 2020

Also remove "skin":"0","skin_admin":"0" in base.sql in the installation folder.

Co-authored-by: Quy <quy@fluxbb.org>
dgrammatiko and others added 2 commits May 29, 2020 18:12
Co-authored-by: Quy <quy@fluxbb.org>
Co-authored-by: Quy <quy@fluxbb.org>
@Quy
Copy link
Contributor

Quy commented May 29, 2020

I have tested this item ✅ successfully on fd7f22b


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/29259.

$skin = Folder::exists(JPATH_ROOT . '/media/vendor/tinymce/skins/ui/' . $skin) ? $skin : 'oxide';
if (is_dir($path))
{
$directories = glob($path . '/*', GLOB_ONLYDIR);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why accept a random folder name? This reintroduces confusing behavior from before #25493.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will only accept one in this case so there shouldn't be an issue now. People seem to like naming things (eg match the template name), hardcoding this is an unnecessary limitation. Maybe the php here is a bit weird because all we want to do here is check if there is a folder in that path and if so then we'll use it.

@Quy
Copy link
Contributor

Quy commented May 29, 2020

I have tested this item ✅ successfully on 7ead8a6


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/29259.

@bonzani
Copy link

bonzani commented Jun 3, 2020

I have tested this item ✅ successfully on e6c9e69


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/29259.

@richard67
Copy link
Member

@Quy Could you repeat your test? Just to be safe aftert the last commit.

@dgrammatiko
Copy link
Contributor Author

dgrammatiko commented Jun 3, 2020

@Quy please hold your test
@richard67 I think I have messed up the sql's here. Let me fix that

Edit: No the changes from NULL to ' ' is what's in the current branch

@Quy this is fine you can proceed with the testing (thanks)

@richard67
Copy link
Member

@dgrammatiko Looks ok now.

@Quy
Copy link
Contributor

Quy commented Jun 3, 2020

I have tested this item ✅ successfully on e6c9e69


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/29259.

@Quy
Copy link
Contributor

Quy commented Jun 3, 2020

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/29259.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Jun 3, 2020

if (is_array($directories))
{
$skin = Uri::root(true) . ($isAdmin ? '/administrator' : '')
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just Uri::base()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question 😃 (probably I forgot that this was an option)

@wilsonge
Copy link
Contributor

wilsonge commented Jun 7, 2020

So I'm in favour of this change overall for overriding a skin at the template level rather than placing into the media directory.

However I think that probably we want to keep the options in the UI.

For two reasons - first of all we've lost the ability to use the oxide-dark default tiny theme. Which for dark skinned templates is probably just annoying that they would then have to keep things synched. Secondly I guess it allows different themings for the different levels (90% of the time probably not a major issue but when you're showing limited options to people I guess situationally could be useful)

@dgrammatiko
Copy link
Contributor Author

dgrammatiko commented Jun 7, 2020

Secondly I guess it allows different themings for the different levels

From a consistency and design integrity point of view people shouldn't roll different skins in a single template. The idea is that the skin is an integral part of the used template. But this is still possible, @Fedik already showcased how someone can override any (that also covers the skin) tinyMCE option. Personally I'm in favour of options that:

  • are covering the common, 80% of the use cases. Chasing always the 20% proved to be problematic in Joomla.
  • are not allowing/promoting a bad pattern (in this case breaking the UI consistency)

Which for dark skinned templates is probably just annoying that they would then have to keep things synched

I'm pretty sure (I had a little chat with Charlie on this one) proper front enders they will use http://skin.tiny.cloud/t5/ to provide a concrete skin that aligns with their templates colours, etc. So there isn't much to worry about here. (Their tool allows to import JSON data for the compilation not to mention that you can have the source and run it as a step of the rest of the building tools)

Anyways, that was my personal take here, if you insist on having back the options then let's get them back...

@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Jul 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Language Change This is for Translators
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants