Skip to content

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

Merged
cpsievert merged 6 commits intorstudio:mainfrom
gsmolinski:modal-bootstrap-3
Jan 12, 2023
Merged

Change size 'xl' of modalDialog to 'l' if Bootstrap 3#3593
cpsievert merged 6 commits intorstudio:mainfrom
gsmolinski:modal-bootstrap-3

Conversation

@gsmolinski
Copy link
Copy Markdown
Contributor

@gsmolinski gsmolinski commented Feb 24, 2022

(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 gsmolinski changed the title Modal bootstrap 3 Change size 'xl' of modalDialog to 'l' if Bootstrap 3 Feb 24, 2022
@cpsievert
Copy link
Copy Markdown
Collaborator

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

@gsmolinski
Copy link
Copy Markdown
Contributor Author

gsmolinski commented Jun 14, 2022

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

Comment thread R/modal.R Outdated
adds note about how to switch to Bootstrap 4+ with bslib

Co-authored-by: Carson Sievert <cpsievert1@gmail.com>
@gsmolinski
Copy link
Copy Markdown
Contributor Author

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

@cpsievert
Copy link
Copy Markdown
Collaborator

yes that'd be great

@gsmolinski
Copy link
Copy Markdown
Contributor Author

Thank you for review, manual is updated.

Comment thread NEWS.md Outdated
@cpsievert cpsievert merged commit 20f8a18 into rstudio:main Jan 12, 2023
@gsmolinski gsmolinski deleted the modal-bootstrap-3 branch January 13, 2023 11:26
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.

2 participants