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
J3.5: Editor buttons can no longer define popup size #8378
Comments
@nonumber if I am not wrong the above code will result to a 100x100 window, can you point me to a plugin that uses this code so I can come up with a patch? |
Try the editor-xtd buttons of for instance NoNumber Tabs or Sourcerer. |
I guess we can expand this part of tinyMCE to take care of the javascript calculation // Get the modal width/height
if ($options)
{
preg_match('/x:\s*+\d{2,4}/', $options, $modalWidth);
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);
} |
closing as we have a PR. Thanks. |
Asking to revert is equal to proposing DO NOT MAKE ANY PROGRESS. I would expect a patch from a top notch coder instead of asking the project to hold back |
Why close this? There is a much greater issue here: I am not saying there shouldn't be any progress. |
Feel free to reopen if needed. Thanks |
@dgt41 gt41
No, incorrect,it opens the window to max size minus 100 pxs. |
That is fixed For the part of the width/height calculation I pointed where the changes should be made, feel free to make a pr |
Imho, looks like the PR indeed wasn't B/C. The PR you proposed does fix the article button, but it doesn't fix the buttons for 3rd party buttons. |
Well, I guess 3rd party extensions shouldn't be referencing the SqueezeBox script directly, but limit to only using the available jModal scripts. |
we can bind that function... |
Just to deal with false expectations: No, I won't be creating any PRs/fixes for Joomla core. |
But you do expect that Joomla will deal with every odd thing any developer is doing out there? Cool! I think I won’t do any more PRs as well... |
Sorry, but the discussion is going nowhere. Never mind. I stopped caring. I'll just work around any new issues Joomla 3.5 throws at us. |
It's not an odd thing when a core button itself becomes an issue with it. |
@Bakual the core buttons operate correctly if #8380 is applied. The only part that is failing is the part where modal dimensions do have some javascript calculation. But even that is easy to fix expanding the preg_match to first find the word window if that is true then return the whole thing after the semicolon else do the part that is already there. |
Isn't the fact that a PR is needed to change that layout meaning it's not B/C? 3rd party extensions which do similar things would need to apply a similar patch to their plugins and components so they work again. |
We have to keep mootools itself until 4.0 anyway. And yes, maybe it also means to use the mootools based modals if there is no B/C way of changing it. |
@Bakual the change in the layout is irrelevant with the problem, this was just an improvement of the current code. The lines that correct the problem are within tinyMCE php file: function jModalClose() {
tinyMCE.activeEditor.windowManager.close();
} But if we have to satisfy that someone put in the options of the modal some strange code to recalculate the width/height, I guess we have to stay with mootools. One question though, these plugins don’t also work on mobile devices? If they do so (responsive kinda way, css), what is all this fuzz? |
my 5 cent 👀 |
Please don't judge code from 3rd parties. We gave a B/C promise and we broke that. In fact in at least two ways. Imho, the feature of having the buttons as native editor buttons as well as having bootstrap modals, while being very nice, isn't worth a broken plugin. It's only a visual/UX thing here. But if that UX improvement actually turns out to break functionality, the improvement goes down the drain very fast :-) |
Can we have a quick win by only moving the core buttons to the toolbar and On 11 November 2015 at 21:47, Thomas Hunziker notifications@github.com
Brian Teeman |
Disagree greatly with that! it is also one less javascript framework and another (modal) script with total weight over 100kb (compressed) and an additional latency to the page load. My first approach was almost what @brianteeman suggests, but without the option for 3pd to opt-in, #7152. This will keep things safe for every other button (but with mootools in place) |
If I read the code correct it looks to me like you just fall back to a fixed dimension when the word "window" is found within the dimension option? Or do I read the code wrong? |
btw, I have doubt that we need to apply old hacks for new modal... because it is totally different things. Complicated 😄 |
I am gonna close #8380 as there is another pr #8366 which also restores some other things I did with the drag and drop PR. So please test that one #8366 @Fedik what I did was search for the word window and if found fall back to 800x600, which works fine for me. Also can you check the SqueezeBox.close override, I wasn’t able to test it, I am on the road the few last days... |
@nonumber thank you for reporting the issue.
I guess this proves we can never test enough.
This is definitely welcomed. As @Bakual said, if we can't fix it before the RC, I am afraid we have to revert it, so let's see what we can do. We have a new PR #8366 that we can test. @Fedik if Mootools uses eval, we can probably do the same thing. It isn't worse than what is already there, if that is the only thing we need to take from there. If it works now, it should also work in 3.5.0, extension devs shouldn't have to change their code. At least not in the 3.x series. This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8378. |
Since we have a PR at 8366, I am going to close this one to keep the discussion in 1 place. I have posted screenshots of @nonumber findings in the other PR. This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8378. |
Since the implementation of #7170 the editor buttons have no control anymore over the size of the modal popup.
You could do this before in the editors-xtd plugins render method:
$button->options = "{handler: 'iframe', size: {x:window.getSize().x-100, y: window.getSize().y-100}}";
This would get placed in the html
rel
attribute used by the Squeezebox modal.Now the
option
is completely disregarded.These plugins can also be having different scripts inside the modal to control the popup, like closing it on click.
These all now fail ungracefully
Even the core
Article
button doesn't work anymore (aside from the fact that it looks bad because of the missing padding in the modal).It will insert the article, but not close the modal, with js error:
This is a serious B/C issue and this PR needs to get reverted, IMO.
The text was updated successfully, but these errors were encountered: