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

Redo Fix modal scrolling #9140 + fix for Batch #9817

Merged
merged 8 commits into from Apr 12, 2016

Conversation

Projects
None yet
6 participants
@JoomliC
Copy link
Contributor

JoomliC commented Apr 9, 2016

Redo Fix modal scrolling #9140 + fix for Batch
The scrolling issue on iOS is a known issue of BS2 and BS3 modals.

Pull Request too for Batch overflow Issues: #9683, #9807, #9772, #9551, #9709.

Summary of Changes

  • remove css of PR #9140 previously added to fix scrolling on iOS after addition of BS modals in 3.5.0 RC (now managed by main.php layout of the BS modal).
  • change modal event handler to shown.bs.modal and hide.bs.modal (http://getbootstrap.com/javascript/#modals-events) to be able to check height when modal has been made visible to the user, and then apply some changes (max-height and overflow) only if window viewport height is too small for the rendered modal.
  • apply max-height and over-flow when needed on modal body, to allow scrolling inside a modal when on small devices.

Testing Instructions

  • To be tested on staging (on 3.5.1 stable, you will have all the admin submenus disappeared)
  • test on as many browsers and devices as you can (desktop, tablet, mobile, FF, IE, Chrome, Safari...)
  • test all modal types : User, Batch (test dropdown!), article Versions (iframe url)...
  • test on admin and frontend, with Isis, Protostar, and other templates...
    You should be able to scroll inside and use modals on all devices.

Details on browsers, platform and devices used for testing are welcomed ;-)

Thanks!

@smz

This comment has been minimized.

Copy link
Contributor

smz commented Apr 9, 2016

Hi Cyril!

At first sight (haven't tested yet...) this looks like a solid implementation! Thanks!

Unhappily next week I'll be out and probably I would not have much time for testing. Let's see if I can do something tonight or tomorrow...

I don't have any "iAnything", nor Android smartphones for testing: just my oooold faithfull Blackberry, so I think my testing will be limited to desktop browsers.

Regards,

Sergio

P.S.: ... evoking @dgt41 in here, if not already done... Hi Dimitri! 😜

@smz

This comment has been minimized.

Copy link
Contributor

smz commented Apr 9, 2016

Cyril,

I found some time to test this and it works: I've tested it against current staging using the Menu Batch modals and resizing my viewport using FF "Responsive Design View".

There is only one minor "glitch" that I don't know if you deem worth to be fixed or not: while resizing the viewport from BIG to small works perfectly (content is resized, internal scrollbar appears), when resizing from small to BIG the height of the internal div is not adjusted to the new available "real estate" and thus the internal scrollbar stays there. No big deal, but I thought you would like to know.

Let me know if you think you'll fix this too: in case I'll re-test, if not I'll give you my 👍

I think more testing performed by people having access to real iOS devices is preferable...

Thanks again!

@JoomliC

This comment has been minimized.

Copy link
Contributor

JoomliC commented Apr 9, 2016

Thanks for test and feedback @smz !

There is only one minor "glitch" that I don't know if you deem worth to be fixed or not: while resizing the viewport from BIG to small works perfectly (content is resized, internal scrollbar appears), when resizing from small to BIG the height of the internal div is not adjusted to the new available "real estate" and thus the internal scrollbar stays there. No big deal, but I thought you would like to know.

So, you have resize your window, for testing purpose?
In real usage, not sure a user will open a modal, and then resize its window... ;-)

We could of course add a resize() handler and re-calculate height on resize, but there will be performance issue especially in IE and webkit browsers (Chrome, Safari...) many resize events fire as long as the user continues resizing the window. (not in FF or Opera, where event is fired at end of resizing).
We could add a timeout to the resize event to lower this effect, but still a bit of performance issue.

The fact is BS2 and BS3 modal has a built issue (nice looking, but not easy to manage in a responsive environnement...).

The main purpose here is just to be able to use the BS modal in all screen sizes (that is not possible on small screen with original BS modal).
In fact, a fix for BS modal, not a refactory ;-)

I don't have any "iAnything", nor Android smartphones for testing: just my oooold faithfull Blackberry, so I think my testing will be limited to desktop browsers.

All tests are welcomed! And... i don't have a Blackberry, so yours is needed! 👍

@smz

This comment has been minimized.

Copy link
Contributor

smz commented Apr 10, 2016

All tests are welcomed! And... i don't have a Blackberry, so yours is needed!

hmm... I have to figure out how to convince my Blackberry to go to my dev environment in my LAN... I can't to do that simply by IP address as my dev environment hosts several sites, and I don't think my Blackberry has the equivalent of an /etc/hosts

I was thinking... would it be handy if I temporarily offer to you a cPanel on my VPS for testing that? I don't have the time to make a Joomla installation, but I can quickly create the cPanel, give you the access credentials and then you can manage to install Joomla on that and give Joomla admin rights to a few trusted testers... What do you think? This can eventually be done only tomorrow as by Monday I'll be on the go...

@smz

This comment has been minimized.

Copy link
Contributor

smz commented Apr 10, 2016

... and:

In real usage, not sure a user will open a modal, and then resize its window... ;-)

agreed!!

@franz-wohlkoenig

This comment has been minimized.

Copy link

franz-wohlkoenig commented Apr 10, 2016

I have tested this item 🔴 unsuccessfully on f1e0ab5

Modal Window stay to small in Height. You have to scrool in Window and Dropdown-List:
screen shot 2016-04-10 at 08 11 40

Test only on Mac/Firefox and stopp testing cause of Behaviour.


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

@brianteeman

This comment has been minimized.

Copy link
Contributor

brianteeman commented Apr 10, 2016

I have tested this item successfully on f1e0ab5

Tested on multiple browsers and devices - all good


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

@brianteeman

This comment has been minimized.

Copy link
Contributor

brianteeman commented Apr 10, 2016

@franz-wohlkoenig
1.did you clear the browser cache as there has been a css change
2. what happens when you click on the select boxes. It should look like this
screen shot 2016-04-10 at 05 48 22


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

@franz-wohlkoenig

This comment has been minimized.

Copy link

franz-wohlkoenig commented Apr 10, 2016

@brianteeman Browser-Cache is cleared. A screen recording at https://youtu.be/SxfQaw8iP14 show what happens when click on select boxes.

@brianteeman

This comment has been minimized.

Copy link
Contributor

brianteeman commented Apr 10, 2016

@brianteeman

This comment has been minimized.

Copy link
Contributor

brianteeman commented Apr 10, 2016

I have tested this item 🔴 unsuccessfully on f1e0ab5

Changing my test result to failed.
@JoomliC the pr fixes the modal but I discovered after applying it that all the admin submenus no longer appear


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

@franz-wohlkoenig

This comment has been minimized.

Copy link

franz-wohlkoenig commented Apr 10, 2016

I have tested this item successfully on f1e0ab5

Test on Module, Menu

  • Mac 10.11.4: Firefox45, Safari9.1, Chrome49
  • Windows Server 2008: Firefox43, IE11

It works now cause i had forgot to reapply Patch.


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

@brianteeman

This comment has been minimized.

Copy link
Contributor

brianteeman commented Apr 10, 2016

@franz-wohlkoenig can you test the menu dropdown submenus AFTER applying the patch. For me they stopped working


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

@smz

This comment has been minimized.

Copy link
Contributor

smz commented Apr 10, 2016

@brianteeman which submenu? This?
capture

@franz-wohlkoenig

This comment has been minimized.

Copy link

franz-wohlkoenig commented Apr 10, 2016

I have tested this item 🔴 unsuccessfully on f1e0ab5

Revert Test result like @brianteeman cause of lost of Submenues in Admin-Template.


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

@JoomliC

This comment has been minimized.

Copy link
Contributor

JoomliC commented Apr 10, 2016

@smz with batch on com_menus ? or else... tested, but no issue...

@franz-wohlkoenig do you have tested against current staging ? As relative PR was merged 4 days ago.
If you test it on a staging older or 3.5.1 stable, you will have submenu issue
Thanks to confirm about the staging version used (today!) ;-)

@smz

This comment has been minimized.

Copy link
Contributor

smz commented Apr 10, 2016

@JoomliC actually my issue doesn't have anything to do with resize: sometimes it happens, sometimes not... both with and w/o your patch. Worst glitches to debug, I know... time dependend... :-/

If I were you I'd simply disregard and add this to the "Unexpected Expected Behaviours" list...

@brianteeman

This comment has been minimized.

Copy link
Contributor

brianteeman commented Apr 10, 2016

I have tested this item successfully on f1e0ab5

Tested with staging and all good


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

@franz-wohlkoenig

This comment has been minimized.

Copy link

franz-wohlkoenig commented Apr 10, 2016

@JoomliC Update Channel is "Testing" > "no Updates available". So i'm on 3.5.1. New Data fetched, Cache cleared after Patch applied.

@brianteeman

This comment has been minimized.

Copy link
Contributor

brianteeman commented Apr 10, 2016

To get the lastest staging you need to download the full install from here
and install as a NEW site
https://github.com/joomla/joomla-cms/archive/staging.zip

On 10 April 2016 at 16:27, Franz Wohlkönig notifications@github.com wrote:

@JoomliC https://github.com/JoomliC Update Channel is "Testing" > "no
Updates available". So i'm on 3.5.1. New Data fetched, Cache cleared after
Patch applied.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#9817 (comment)

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/

@franz-wohlkoenig

This comment has been minimized.

Copy link

franz-wohlkoenig commented Apr 10, 2016

thanks @brianteeman, will report.

@franz-wohlkoenig

This comment has been minimized.

Copy link

franz-wohlkoenig commented Apr 10, 2016

I have tested this item successfully on f1e0ab5

Submenues are shown also as the Dropdown-List in an Modal Window on Joomla! 3.5.2-dev


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

@brianteeman

This comment has been minimized.

Copy link
Contributor

brianteeman commented Apr 10, 2016

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC label Apr 10, 2016

@brianteeman brianteeman added this to the Joomla 3.5.2 milestone Apr 10, 2016

@franz-wohlkoenig

This comment has been minimized.

Copy link

franz-wohlkoenig commented Apr 11, 2016

@brianteeman can you please tell me how to know when there is a new "latest staging" at https://github.com/joomla/joomla-cms/archive/staging.zip?

@brianteeman

This comment has been minimized.

Copy link
Contributor

brianteeman commented Apr 11, 2016

@franz-wohlkoenig every time there is a new pul request merged the link you have wlll generate a new copy of staging

@franz-wohlkoenig

This comment has been minimized.

Copy link

franz-wohlkoenig commented Apr 11, 2016

thanks @brianteeman for Explanation.

@JoomliC

This comment has been minimized.

Copy link
Contributor

JoomliC commented Apr 11, 2016

Thanks @brianteeman @smz @franz-wohlkoenig for testing!

@franz-wohlkoenig to add an addtional info, there was an issue on 3.5.1 to apply this PR because one other PR was already merged in 3.5.2-dev (staging), including changes in Isis template.css.
This is not always the case, but by appling this PR on 3.5.1 and not on staging, the issue was a partial integration of the other PR (and so, was broken) ;-)

@franz-wohlkoenig

This comment has been minimized.

Copy link

franz-wohlkoenig commented Apr 11, 2016

Thanks @JoomliC So its ok to test like now on 3.5.1. But if PR requier "latest staging" it means delete Testsite & install latest staging as new Site.

@JoomliC

This comment has been minimized.

Copy link
Contributor

JoomliC commented Apr 11, 2016

@franz-wohlkoenig or just to replace the current install files by staging files. ;-)

@franz-wohlkoenig

This comment has been minimized.

Copy link

franz-wohlkoenig commented Apr 11, 2016

a ftp-upload is easier*g, thanks @JoomliC

@rdeutz rdeutz merged commit df5abe6 into joomla:staging Apr 12, 2016

2 checks passed

JTracker/HumanTestResults Human Test Results: 3 Successful 0 Failed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@joomla-cms-bot joomla-cms-bot removed the RTC label Apr 12, 2016

@rdeutz rdeutz modified the milestones: Joomla 3.5.2, Joomla! 3.6.0 May 1, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment