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 scrollable property to modal #64

Merged
merged 3 commits into from
Nov 19, 2020
Merged

Conversation

DaleCam
Copy link

@DaleCam DaleCam commented Oct 7, 2020

Adds optional property scrollable to modal. Fixes issue #61
Important!:
I wasn't able to figure out how to build/serve this to visually test it. Feedback on how to build so i can test locally would be appreciated, however the code is covered by new unit tests I added and they are working as expected.

Dale Cameron added 2 commits October 7, 2020 14:47
@DaleCam DaleCam changed the title fix - add scollable property to modal Add scrollable property to modal Oct 7, 2020
@Morgul
Copy link
Owner

Morgul commented Oct 8, 2020

The implementation looks reasonable. Regarding building... yeah, I'm not even sure I can currently build the project; I've gone through two machines since I last tried.

This weekend, I'll put some effort into getting it to build and see if I can't get a release out with these changes.

@Morgul Morgul self-requested a review October 8, 2020 18:33
@Morgul Morgul self-assigned this Oct 8, 2020
@Morgul Morgul added this to the v3.0.5 milestone Oct 8, 2020
@Morgul Morgul linked an issue Oct 8, 2020 that may be closed by this pull request
Copy link

@Drumstix42 Drumstix42 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good. Would love to see this make it into the code

As an aside:
It might be good to [also] allow the flexibility of custom class names to make it onto the same element as .modal-dialog. I'm all for easy to use property booleans, but there are some other helpers, and sometimes you may run into a situation where you'd like to supplement these with an additional class name. For a basic example though, Bootstrap 4 documents notes you can do things such as:

<!-- Vertically centered scrollable modal -->
<div class="modal-dialog modal-dialog-centered modal-dialog-scrollable">
  ...
</div>

Edit
For anyone reaching this comment, another user pointed out you can "hijack" the "size" value and put in other class names, as it's just string that gets placed in the appropriate element.
#58 (comment)

@Morgul Morgul merged commit c094c72 into Morgul:master Nov 19, 2020
@Drumstix42
Copy link

@Morgul Will you be making a new release/tag now that this is merged and completes the related Milestone?

@Morgul
Copy link
Owner

Morgul commented Nov 19, 2020

New version should be on npm. Let me know if there's issues, it's been two years since I've done this with this project, and nothing about it's standard. 😐

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

Successfully merging this pull request may close these issues.

Modal: Add support for .modal-dialog-scrollable
3 participants