Skip to content

Fix modals with lengthy content #101

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

Closed
wants to merge 1 commit into from
Closed

Fix modals with lengthy content #101

wants to merge 1 commit into from

Conversation

nervetattoo
Copy link
Contributor

Modals would grow out of the document window when a lot of content was added to it. This fix makes it scale in max-height.
It adds the same margin as found on top, to the bottom.

From:
screen shot 2014-08-25 at 10 10 15

To:
screen shot 2014-08-25 at 10 10 32

@nervetattoo nervetattoo changed the title Fix modals with length content Fix modals with lengthy content Aug 26, 2014
@nervetattoo
Copy link
Contributor Author

I've gone back and forth a few times before landing on this implementation.
Now it inserts a faux modal in the DOM first in order to measure its top property, then it uses that to set a max-height on the new modal.

Initially I wanted to measure this on the actual modal, but animations make it hard to time the right moment, so this implementation turns out more lightweight.

@circlingthesun
Copy link
Contributor

Vanilla Foundation does not make the modal scrollable but instead lets it expand and appear at the correct position. Have you had a look at #89? I think if you make it scrollable it should be an option instead of default behaviour.

@jbrowning
Copy link
Member

Agreed with @circlingthesun. This behavior should be optional. Some apps may prefer to scroll the viewport instead of scrolling the modal content.

@nervetattoo
Copy link
Contributor Author

You are right. The default behaviour should be the same as in foundation. I was just so damn sure VF made it scrollable inside ;)
I don't think two behaviours should be implemented, so its better to just get #89 merged. Closing this for now.

@nervetattoo nervetattoo mentioned this pull request Aug 27, 2014
@jbrowning
Copy link
Member

👍 thanks

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.

3 participants