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

tinyMCE DRY for the inline script #8366

Merged
merged 10 commits into from Nov 18, 2015
Merged

tinyMCE DRY for the inline script #8366

merged 10 commits into from Nov 18, 2015

Conversation

dgrammatiko
Copy link
Contributor

DRY

Few lines are repeated on the 3 different modes. We don’t have to…

Also instead of using tinyMCE.activeEditor we target exactly the editor we want.

Testing

Use tinyMCE as your editor. Try all different modes (simple, advanced, extended).

@Fedik
Copy link
Member

Fedik commented Nov 12, 2015

I got Uncaught TypeError: window.parent.jInsertEditorText is not a function when try select something in the modal (from xtd buttons)
seems public function onGetInsertMethod($name) never called .. strange

other things seems work fine 😄

@dgrammatiko
Copy link
Contributor Author

@Fedik normality restored...

@Fedik
Copy link
Member

Fedik commented Nov 13, 2015

one more:
When click on "Read More" button Uncaught TypeError: Cannot read property 'getContent' of null

@roland-d
Copy link
Contributor

Using the extension sliders, I get a different popup clicking on the button. This is in J 3.4.5:
image

and this is in 3.5.0-dev:
image

@Fedik mentioned in #8378 this is due to Mootools using eval for the code "{handler: 'iframe', size: {x:window.getSize().x-100, y: window.getSize().y-100}}" perhaps we can do the same?

@roland-d
Copy link
Contributor

We are in need of some padding above the Page title field:
image

Since the article modal needs some padding as well, perhaps we need to a padding all around of let's say10px.

preg_match('/y:\s*+\d{2,4}/', $options, $modalHeight);
$modalWidth = filter_var(implode("", $modalWidth), FILTER_SANITIZE_NUMBER_INT);
$modalHeight = filter_var(implode("", $modalHeight), FILTER_SANITIZE_NUMBER_INT);
preg_match('/\s*+(window)/', $options, $matches);
Copy link
Member

Choose a reason for hiding this comment

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

maybe better idea is to move it to javascript and parse on the client side, with Function constructor:

var getOptions = new Function("return {handler: 'iframe', size: {x:window.getSize().x-100, y: window.getSize().y-100}}");
var options = getOptions();
var width = options.size && options.size.x ?  options.size.x : 0;
var height = options.size && options.size.y ?  options.size.y : 0;

but I have no idea what to do with bootstrap modals, where the width/height need to be set in the iframe layout 🙊

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The buttons are not available with bootstrap modal, yet!

Copy link
Member

Choose a reason for hiding this comment

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

ignore my previous comment,
I totally missed that getSize() in window.getSize() comes from mootools,
nor eval nor my suggestion will not work here without mootools loaded

Copy link
Contributor

Choose a reason for hiding this comment

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

@Fedik we should be able to extract or copy the logic that calculates the modal size or not?

Copy link
Member

Choose a reason for hiding this comment

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

hm,
I see only 2 way (revert changes I not count):

  • or load mootools
  • or create own method window.getSize like:
window.getSize = window.getSize || function(){
 return {x: jQuery(window).width(), y: jQuery(window).height() };
};

(but no idea where to place this code)

then it can work with that suggestion https://github.com/dgt41/joomla-cms/pull/27

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Fedik still got errors, the problem is that we need to revaluate the width and height since it is not just the window size (we should parse the integer and subtract from the window width). I think hardcoding these values is way easier...

Copy link
Member

Choose a reason for hiding this comment

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

which errors you got?
here "...{x:window.getSize().x-100, y: window.getSize().y-100}.." seems work fine with Fedik@faf1fa5

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Fedik I am stupid, I had edited the line "...{x:window.getSize().x-100, y: window.getSize().y-100}.." that’s why I was getting an error 🙈
This works fine, thanks again!

@roland-d
Copy link
Contributor

I have tested this item ✅ successfully on 2b774e6

After applying the patch, the modals show up in the desired size and the padding is applied.


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

@Fedik
Copy link
Member

Fedik commented Nov 18, 2015

I have tested this item ✅ successfully on 2b774e6


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

@dgrammatiko dgrammatiko changed the title tinyMCE DRY for the inline script tinyMCE DRY for the inline script Nov 18, 2015
@dgrammatiko
Copy link
Contributor Author

@joomla-cms-bot are you sleeping?

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Nov 18, 2015
@zero-24
Copy link
Contributor

zero-24 commented Nov 18, 2015

Cool @dgt41 i also got worried about him. Thanks for wake up @joomla-cms-bot :)

rdeutz added a commit that referenced this pull request Nov 18, 2015
tinyMCE DRY for the inline script
@rdeutz rdeutz merged commit c170fae into joomla:staging Nov 18, 2015
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Nov 18, 2015
@dgrammatiko dgrammatiko deleted the tinyMceRedo branch November 18, 2015 19:12
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.

None yet

7 participants