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

css: newsletter-signup #583

Merged
2 commits merged into from
Mar 17, 2016
Merged

css: newsletter-signup #583

2 commits merged into from
Mar 17, 2016

Conversation

LoganArnett
Copy link
Contributor

Found this in relation to #57, the iframe for the newsletter signup has a large amount of empty whitespace underneath it with a default height="500" on both desktop and mobile. Just changed to max-height to avoid this.

@ghost
Copy link

ghost commented Mar 16, 2016

Travis build passed 👍

@LoganArnett LoganArnett changed the title fix/newsletter-signup css: newsletter-signup Mar 16, 2016
@williamkapke
Copy link
Contributor

Is this related to #578?

@LoganArnett
Copy link
Contributor Author

@williamkapke I dont believe so, this is for when the form is present not after it has been filled out and disappears

@lpinca
Copy link
Member

lpinca commented Mar 17, 2016

@LoganArnett what you are doing here is changing the HTML height attribute to max-height which is not a valid HTML attribute.

With this change the iframe will display with its default height which is 150px.
This is not sufficient to display the full iframe contents on screens which are between 480px and 580px wide unless we are ok to have a vertical scroll bar.

Setting max-height in the style attribute also does not help.

That said, reducing the current value from 500 to 300 should be safe and partially fix the issue.

@LoganArnett
Copy link
Contributor Author

Yea I do not know why I changed it that way, yesterday was a long day haha I can update it to the 300px

@lpinca
Copy link
Member

lpinca commented Mar 17, 2016

LGTM.

@ghost
Copy link

ghost commented Mar 17, 2016

yup, LGTM from me as well

This pull request was closed.
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