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

Add viewport units options for modals #10388

Merged
merged 10 commits into from May 10, 2016

Conversation

cyrezdev
Copy link
Contributor

@cyrezdev cyrezdev commented May 10, 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 Changes Styles from Modals for higher "Shadowbox"-Modals #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.

@andrepereiradasilva
Copy link
Contributor

I have tested this item ✅ 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.

@andrepereiradasilva
Copy link
Contributor

BTW also tested in Chrome and IE10

@cyrezdev
Copy link
Contributor Author

cyrezdev commented May 10, 2016

@andrepereiradasilva Thanks for testing and feedback! 👍
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)

@zero-24
Copy link
Contributor

zero-24 commented May 10, 2016

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

@brianteeman
Copy link
Contributor

@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)

@zero-24
Copy link
Contributor

zero-24 commented May 10, 2016

Thanks! @brianteeman

@RoterNagel
Copy link
Contributor

RoterNagel commented May 10, 2016

I have tested this item ✅ successfully on 545a9a3

And you choose the menu type modal for testing ❤️ 😉

Should I run a test via browserstack, too?


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

@MATsxm
Copy link

MATsxm commented May 10, 2016

I have tested this item ✅ successfully on 545a9a3

thanks


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

@zero-24
Copy link
Contributor

zero-24 commented May 10, 2016

RTC based on tests


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label May 10, 2016
@brianteeman brianteeman added this to the Joomla 3.6.0 milestone May 10, 2016
@cyrezdev
Copy link
Contributor Author

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

Thanks for testing!

@rdeutz rdeutz merged commit f95d899 into joomla:staging May 10, 2016
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label May 10, 2016
@dgrammatiko
Copy link
Contributor

Great work @JoomliC Thank you!

@cyrezdev
Copy link
Contributor Author

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)

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

9 participants