-
-
Notifications
You must be signed in to change notification settings - Fork 41
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
Conversation
Adds optional property scrollable to modal. Fixes issue Morgul#61
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. |
There was a problem hiding this 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 Will you be making a new release/tag now that this is merged and completes the related Milestone? |
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. 😐 |
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.