shiny icon indicating copy to clipboard operation
shiny copied to clipboard

Change size 'xl' of modalDialog to 'l' if Bootstrap 3

Open gsmolinski opened this issue 2 years ago • 5 comments

(Problem mentioned also in the issue #3631)

Motivation

modalDialog with size set to xl in Bootstrap 3 is not supported (class 'modal-xl' is not supported). Problem is that this is a default Bootstrap version for Shiny, there is also no information about this issue in documentation. Now the behavior may be surprising for the user if working with Bootstrap 3 - user by chosen size xl probably wants the biggest modal dialog, but gets medium size. My intention was to change the size into l (large, 'modal-lg') in this case.

What was changed

  • I have changed documentation for parameter size with information that in Bootstrap 3 size xl will be changed into l as xl is not supported in Bootstrap 3.
  • I have added JS code to (1) check the version of Bootstrap; (2) if Bootstrap 3 and first child has class 'modal-xl', remove this class and add class 'modal-lg'.
  • I have added entry to the NEWS.md to the 'Bug fixes'.

Risks

  • I'm not sure if the code used to check the Bootstrap version is reliable.
  • I'm taking the first child of modal dialog to change the class - if the structure will change, the code won't work.
  • Tested only manually (I have run devtools::check() and devtools::test(), get some fails / warnings, but I think not related to my changes).

Other comments

  • I think the added code should be removed once the Shiny will get as a default Bootstrap 4 or higher version.

gsmolinski avatar Feb 24 '22 13:02 gsmolinski

Thanks for PR. I'm not particularly keen on shimming xl to also work with Bootstrap 3. I'd rather just document it properly

cpsievert avatar Jun 14 '22 15:06 cpsievert

I understand; I have prepared another commit, this one only documents that in Bootstrap 3 instead of size xl, size m will be displayed.

gsmolinski avatar Jun 14 '22 19:06 gsmolinski

Should I document() now and commit again because of this update?

gsmolinski avatar Jun 14 '22 20:06 gsmolinski

yes that'd be great

cpsievert avatar Jun 14 '22 20:06 cpsievert

Thank you for review, manual is updated.

gsmolinski avatar Jun 15 '22 06:06 gsmolinski