? Success
Pull Request for # 10070

User tests: Successful: Unsuccessful:

avatar JoomliC
JoomliC
10 May 2016

Pull Request for Issue #10070 .

Based on idea by @RoterNagel about using viewport units for modals, and the proposal of @andrepereiradasilva to create special classes for viewport units.

This PR is full B/C as this one is not changing current behavior. It adds new optional options modalWidth and bodyHeight to be able to set viewport units in the modal constructor.
Note: for old browsers with no viewport unit support, it will just displayed the modal as before.

The Options for modals are then these ones:

  • title (string) : The modal title
  • backdrop (mixed) : A boolean select if a modal-backdrop element should be included (default = true) The string 'static' includes a backdrop which doesn't close the modal on click.
  • keyboard (boolean) : Closes the modal when escape key is pressed (default = true)
  • closeButton (boolean) : Display modal close button (default = true)
  • animation (boolean) : Fade in from the top of the page (default = true)
  • footer (string) : Optional markup for the modal footer
  • url (string) : URL of a resource to be inserted as an iframe inside the modal body
  • height (string) : height of the iframe containing the remote resource
  • width (string) : width of the iframe containing the remote resource
  • NEW bodyHeight (int) : Optional height of the modal body in viewport units (vh)
  • NEW modalWidth (int) : Optional width of the modal in viewport units (vh)

Summary of Changes

  • Changes done on latest Staging (to get the previous modal changes already merged in 3.6.0-dev)
  • New modal options: modalWidth and bodyHeight
  • Add jviewport classes to modals.joomla.less
  • Update responsive-767px-max.joomla.less
  • Run generatecss
  • Integrate new options in the modal layouts (with control for a correct vh value)
  • Add those 2 new options in the menuTypeModal for test and demonstrate B/C

Note: i added viewport options here only for Menu Item Type select for testing purpose. If this PR is accepted, i will then be able to add this to other modals (already updated and merged, and the ones i'm working on and not yet improved). My goal is all modals reviewed for 3.6.0 ;-)

Testing Instructions

  • Apply patch on latest staging
  • Clean cache (to clean template.css in cache)
  • Go to menus > new menu item > click to select a Menu Item Type
  • Verify that now the modal is higher (bodyHeight 70vh) as expected in PR #10070
  • Note: I have set modalWidth to 80vw as the default modal width was already 80% (and in this modal menu-type, seems to be a good width)

Before Patch
Menu Item Type select modal before patch

After Patch
Menu Item Type select modal before patch with viewport units

  • Test other modals (Batch, Messaging > Settings, ...) to confirm B/C, and nothing change for other modals where modalWidth and bodyHeight not specified.
avatar JoomliC JoomliC - open - 10 May 2016
avatar JoomliC JoomliC - change - 10 May 2016
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 10 May 2016
Labels Added: ?
avatar JoomliC JoomliC - change - 10 May 2016
The description was changed
avatar JoomliC JoomliC - change - 10 May 2016
The description was changed
avatar brianteeman brianteeman - change - 10 May 2016
Rel_Number 0 10070
Relation Type Pull Request for
avatar brianteeman brianteeman - change - 10 May 2016
Category Components UI/UX
avatar andrepereiradasilva andrepereiradasilva - test_item - 10 May 2016 - Tested successfully
avatar andrepereiradasilva
andrepereiradasilva - comment - 10 May 2016

I have tested this item :white_check_mark: successfully on 545a9a3

Works as described.

Tested the new menu item type select modal with different window sizes and worked very good.
Tested the other modals (batch, my setting in messages, versions, article select, user notes, user select) and they work as before.

A very nice improvement!
+1 for this modals review for 3.6.0


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

avatar andrepereiradasilva
andrepereiradasilva - comment - 10 May 2016

BTW also tested in Chrome and IE10

avatar JoomliC
JoomliC - comment - 10 May 2016

@andrepereiradasilva Thanks for testing and feedback! :+1:
And as you can see, i've added too a width option for modal!

Note: for old browsers with no viewport unit support, it will just displayed the modal as before ;-)
(so full B/C)

avatar JoomliC JoomliC - change - 10 May 2016
The description was changed
avatar JoomliC JoomliC - change - 10 May 2016
The description was changed
avatar zero-24
zero-24 - comment - 10 May 2016

@brianteeman please add the 3.6.0 milestone here as we have it on the old PR.

avatar brianteeman
brianteeman - comment - 10 May 2016

@zero-24 no need to remind me on each one. I will review them all a few
times a day.
On 10 May 2016 6:14 pm, "zero-24" notifications@github.com wrote:

@brianteeman https://github.com/brianteeman please add the 3.6.0
milestone here as we have it on the old PR.


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

avatar zero-24
zero-24 - comment - 10 May 2016

Thanks! @brianteeman

avatar RoterNagel RoterNagel - test_item - 10 May 2016 - Tested successfully
avatar RoterNagel
RoterNagel - comment - 10 May 2016

I have tested this item :white_check_mark: successfully on 545a9a3

And you choose the menu type modal for testing :heart: :wink:

Should I run a test via browserstack, too?


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

avatar MATsxm MATsxm - test_item - 10 May 2016 - Tested successfully
avatar MATsxm
MATsxm - comment - 10 May 2016

I have tested this item :white_check_mark: successfully on 545a9a3

thanks


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

avatar zero-24 zero-24 - change - 10 May 2016
Status Pending Ready to Commit
avatar zero-24
zero-24 - comment - 10 May 2016

RTC based on tests


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

avatar joomla-cms-bot joomla-cms-bot - change - 10 May 2016
Labels Added: ?
avatar brianteeman brianteeman - change - 10 May 2016
Milestone Added:
avatar JoomliC
JoomliC - comment - 10 May 2016

@RoterNagel i didn't know what browserstack is, but now i know! ;-)
You can of course!

Thanks for testing!

avatar rdeutz rdeutz - reference | f95d899 - 10 May 16
avatar rdeutz rdeutz - merge - 10 May 2016
avatar rdeutz rdeutz - close - 10 May 2016
avatar rdeutz rdeutz - change - 10 May 2016
Status Ready to Commit Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2016-05-10 17:51:24
Closed_By rdeutz
avatar rdeutz rdeutz - close - 10 May 2016
avatar rdeutz rdeutz - merge - 10 May 2016
avatar joomla-cms-bot joomla-cms-bot - close - 10 May 2016
avatar joomla-cms-bot joomla-cms-bot - change - 10 May 2016
Labels Removed: ?
avatar dgt41
dgt41 - comment - 10 May 2016

Great work @JoomliC Thank you!

avatar JoomliC
JoomliC - comment - 10 May 2016

Thanks @RoterNagel @andrepereiradasilva and @MATsxm for testing!

Thanks @dgt41 ! ;-)
Now, time to set it for many modals where needed! (just opened 2 PRs for 3 modals, other to come in the next days)

Add a Comment

Login with GitHub to post a comment