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

Allow different buttons setup for tinyMCE #14417

Closed
wants to merge 3 commits into from
Closed

Allow different buttons setup for tinyMCE #14417

wants to merge 3 commits into from

Conversation

dgrammatiko
Copy link
Contributor

@dgrammatiko dgrammatiko commented Mar 7, 2017

Pull Request complimentary for Issue #14390 .

Summary of Changes

Pass different params for each instance of tinyMCE

Testing Instructions

  • In the global configuration select Tynimce as your default editor.

  • Create a new Editor custom field.

  • Select the Show Buttons as YES!

  • Select in the next input the buttons you want to hide.

  • Save the field.

  • Create a new article

  • Repeat the process but making Show Buttons as NO!

Expected result

Buttons behave according to the given input
screen shot 2017-03-07 at 22 57 58
screen shot 2017-03-07 at 22 58 19

Documentation Changes Required

This might not be B/C compatible (depending the tinyMCE introduced the Joomla.storageOptions)

@zero-24
Copy link
Contributor

zero-24 commented Mar 7, 2017

This might not be B/C compatible (depending the tinyMCE introduced the Joomla.storageOptions)

As this was done in 3.7 this is not a problem right?

@dgrammatiko
Copy link
Contributor Author

I couldn't find the PR for that, thanks for clearing this out

@zero-24
Copy link
Contributor

zero-24 commented Mar 7, 2017

should be this here: #11157 @DGT41

@zero-24 zero-24 added this to the Joomla 3.7.0 milestone Mar 7, 2017
@dgrammatiko
Copy link
Contributor Author

@Fedik can you review the changes here?

@laoneo
Copy link
Member

laoneo commented Mar 8, 2017

Is this ready for testing?

@dgrammatiko
Copy link
Contributor Author

Yup

@laoneo
Copy link
Member

laoneo commented Mar 8, 2017

I have tested this item ✅ successfully on d8d404c


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

@infograf768
Copy link
Member

I have tested this item ✅ successfully on d8d404c

Works nicely.


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

@infograf768
Copy link
Member

RTC


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

@joomla-cms-bot joomla-cms-bot removed this from the Joomla 3.7.0 milestone Mar 8, 2017
@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Mar 8, 2017
@jeckodevelopment jeckodevelopment added this to the Joomla 3.7.0 milestone Mar 8, 2017
@@ -701,7 +701,7 @@ public function onDisplay($name, $content, $width, $height, $col, $row, $buttons
$options['tinyMCE']['default'] = $scriptOptions;

$doc->addStyleDeclaration('.mce-in { padding: 5px 10px !important;}');
$doc->addScriptOptions('plg_editor_tinymce', $options);
$doc->addScriptOptions('plg_editor_tinymce_' . $id, $options);
Copy link
Member

Choose a reason for hiding this comment

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

I am afraid this is not compatible with subform,
There the id will be generated by JS, therefore the new editor instances (added in new row by JS) will not have any defined options while call Joomla.getOptions('plg_editor_tinymce_' + editor.id, {})

@Fedik
Copy link
Member

Fedik commented Mar 8, 2017

@DGT41 I am afraid it will fail in multiple cases

but, there is possibility to override default options for the editor by the editor field name, introduced in #11157, maybe we can use it somehow here. Also 'default' options are cached.

@infograf768
Copy link
Member

shall rtc be removed?

@dgrammatiko
Copy link
Contributor Author

yup, let's figure out another way for this

@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Mar 8, 2017
@joomla-cms-bot joomla-cms-bot removed this from the Joomla 3.7.0 milestone Mar 8, 2017
@infograf768
Copy link
Member

back to pending


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

@coolcat-creations
Copy link
Contributor

Would be great if it will be possible, i ran into this today :) 👍

@dgrammatiko
Copy link
Contributor Author

dgrammatiko commented Mar 10, 2017

@Fedik how about: 54c3a5a

@Fedik
Copy link
Member

Fedik commented Mar 10, 2017

@DGT41 could work, but still not perfect 😉
did you seen, there already test for "whether default parameters loaded"? 😉 https://github.com/dgt41/joomla-cms/blob/5d1b0d995fd96582116a5813884f0ece31ae0f65/plugins/editors/tinymce/tinymce.php#L203

I think here need something like:

// Load default:
if (empty($options['tinyMCE']['default'])) {
  $options['tinyMCE']['default'] = array(/* default, general, params*/)
}

// Load changes for current name
$options['tinyMCE']['fieldName']['toolbar1'] = 'blabla';

@dgrammatiko
Copy link
Contributor Author

@Fedik I know, but the other option needs a lot of refactoring and due to the legacy method is double work. We can postpone it for J4 when we will do the clean up.

@Fedik
Copy link
Member

Fedik commented Mar 10, 2017

I know

then you should note that adding if (!$this->defaultParamsLoaded) make no difference 😉

but the other option needs a lot of refactoring and due to the legacy method is double work.

true, but current solution with _' . $id introduce more mess , I think

I try to check more, this weekend, if will get some time 😉

@Fedik
Copy link
Member

Fedik commented Mar 12, 2017

@DGT41 check this #14520

@dgrammatiko dgrammatiko deleted the tinyMCE_buttons branch March 12, 2017 13:32
@dgrammatiko
Copy link
Contributor Author

there is a new PR #14520

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.

8 participants