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 modals (editors-xtd buttons) are responsive #23091

Merged
merged 6 commits into from Dec 12, 2018

Conversation

Projects
None yet
9 participants
@twister65
Copy link
Contributor

twister65 commented Nov 15, 2018

Pull Request for Issue #18447 and #20350 .

Summary of Changes

We open a modal window in fullscreen for mobile.

Testing Instructions

Log in as administrator with your smartphone.
Select TinyMCE editor in Global Configuration.
Edit an article.
Click on Module, Contact, Menu, Article, Image, Field, Page Break buttons.
Change the orientation of the screen (landscape, portrait) on smartphones.

Expected result

A modal window is opened and fills the screen.
The modals of the xtd plugins are not cut and can be closed.

Actual result

screenshot_image

TinyMCE modals fits on a smartphone.
editors-xtd plugins.

@twister65 twister65 requested a review from laoneo as a code owner Nov 15, 2018

@mbabker

This comment has been minimized.

Copy link
Member

mbabker commented Nov 15, 2018

Use JFactory::getApplication()->client->mobile for mobile device checks instead of hardcoding a list of platforms.

@twister65

This comment has been minimized.

Copy link
Contributor Author

twister65 commented Nov 16, 2018

Use JFactory::getApplication()->client->mobile for mobile device checks instead of hardcoding a list of platforms.

That's what I did first, but it is not fit with a tablet. That's why I only choose smartphones.

@brianteeman

This comment has been minimized.

Copy link
Contributor

brianteeman commented Nov 16, 2018

@mbabker is right - we shouldnt be hardcoding it here and should be using the existing api. We wouldnt want to have to update the list in multiple places. (and anyway this hardcoded list is not complete)

@ivankomlev

This comment has been minimized.

Copy link

ivankomlev commented Nov 16, 2018

I have tested this item successfully on a77176c

Patch works on real devices but solution is perfect as it won't work when you simply change the screen size.


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

@dgrammatiko

This comment has been minimized.

Copy link
Contributor

dgrammatiko commented Nov 17, 2018

@twister65 there is a cleaner way to do this: the mootools modal already has a resize function and the only changes that needs is:

  • on the constructor check if the window width is more than the modal width (then use it else reduce it to something less than the window width, same for the height)
  • make sure that resize follows the same pattern

Solving client side problems in PHP is not really a good idea...

Check https://codepen.io/dgrammatiko/pen/JeyyBx?editors=1010
Lines 148 to 160 on the js part

@twister65

This comment has been minimized.

Copy link
Contributor Author

twister65 commented Nov 17, 2018

Solving client side problems in PHP is not really a good idea...

I totally agree with you

@twister65

This comment has been minimized.

Copy link
Contributor Author

twister65 commented Nov 17, 2018

@dgrammatiko , this is a tinyMCE issue.
#20350 (comment)
It uses its own window manager :

$tempConstructor[] = 'editor.windowManager.open(modalOptions);';

@twister65 twister65 changed the title TinyMCE modals (editors-xtd plugins) are suitable for a smartphone. TinyMCE modals (editors-xtd buttons) are responsive Nov 18, 2018

@twister65

This comment has been minimized.

Copy link
Contributor Author

twister65 commented Nov 18, 2018

@ReLater and @infograf768, please could you test this PR ?
It should fix your issues.

@ReLater

This comment has been minimized.

Copy link
Contributor

ReLater commented Nov 18, 2018

I see a problem with this responsive behavior. Test the Module button with screen width below 769px. Column "Position" is hidden.

I know that's an underlying issue with the modules selection view (CSS classes "hidden-phone" of some columns) but I feel uncertain if I can test successfully then.

@@ -817,7 +817,11 @@ private function tinyButtons($name, $excluded)
// Set width/height
$tempConstructor[] = 'if(modalWidth){modalOptions.width=modalWidth;}';
$tempConstructor[] = 'if(modalHeight){modalOptions.height = modalHeight;}';
$tempConstructor[] = 'editor.windowManager.open(modalOptions);';
$tempConstructor[] = 'var win=editor.windowManager.open(modalOptions);';
if (JFactory::getApplication()->client->mobile)

This comment has been minimized.

@Quy

Quy Nov 18, 2018

Contributor
Suggested change Beta
if (JFactory::getApplication()->client->mobile)
if (JFactory::getApplication()->client->mobile)

This comment has been minimized.

@twister65

twister65 Nov 19, 2018

Author Contributor

Done

@twister65

This comment has been minimized.

Copy link
Contributor Author

twister65 commented Nov 30, 2018

Module modal screenshot with a mobile:
screenshot_module
@ReLater, I think it's very difficult to add other columns with such devices.

@ReLater

This comment has been minimized.

Copy link
Contributor

ReLater commented Dec 9, 2018

@ ReLater, I think it's very difficult to add other columns with such devices.

I propose to remove the CSS classes "hidden-phone" for column "Position" in modal.php view of modules. Maybe in a later pr.

09-12-_2018_21-54-14

@ReLater

This comment has been minimized.

Copy link
Contributor

ReLater commented Dec 9, 2018

I have tested this item successfully on a2523bb


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

@Milglius

This comment has been minimized.

Copy link

Milglius commented Dec 10, 2018

I have tested this item successfully on a2523bb

Thank you


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

@Quy

This comment has been minimized.

Copy link
Contributor

Quy commented Dec 10, 2018

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC label Dec 10, 2018

@Milglius

This comment has been minimized.

Copy link

Milglius commented Dec 10, 2018

I have tested this successfully


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

@mbabker mbabker added this to the Joomla 3.9.2 milestone Dec 12, 2018

@mbabker mbabker merged commit 0a81788 into joomla:staging Dec 12, 2018

4 checks passed

JTracker/HumanTestResults Human Test Results: 2 Successful 0 Failed.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/drone/pr the build was successful
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment