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

Remove hard-coded 'height' #724

Merged
merged 1 commit into from Apr 17, 2018

Conversation

Projects
None yet
3 participants
@MatsPalmgren
Contributor

MatsPalmgren commented Apr 16, 2018

Hard-coded 'height' is bad web design because they cause bugs like these:
https://bugzilla.mozilla.org/show_bug.cgi?id=1453903

Remove hard-coded 'height'
Hard-coded 'height' is bad web design because they cause bugs like these:
https://bugzilla.mozilla.org/show_bug.cgi?id=1453903
@welcome

This comment has been minimized.

welcome bot commented Apr 16, 2018

💖 Thanks for opening this pull request! 💖
Here is a list of things that will help get it across the finish line: - If this is a new or updated CSS interactive example, please ensure that you followed the CSS styleguide - If this is a new or updated JavaScript interactive example, please ensure that you followed the JavaScript styleguide - If your changes affects any of the steps in our contribution docs, please also make the relevant changes there.

@schalkneethling

This comment has been minimized.

Collaborator

schalkneethling commented Apr 16, 2018

Thank you again for the PR @MatsPalmgren - I do agree that setting the height is not ideal. In this case we sometimes need to because out viewport is "fixed", and so we need to make the best use of the space we have.

Removing the height here definitely does not make the example any less effective but, there are surely others where removing the height will not be as simple.

Before I go ahead and merge this one, I wonder if @wbamberg has some additional thoughts?

@wbamberg

This comment has been minimized.

Member

wbamberg commented Apr 16, 2018

No, this seems like a good change to me.

@schalkneethling

This comment has been minimized.

Collaborator

schalkneethling commented Apr 17, 2018

r+ Thanks @MatsPalmgren - Does this mean we can also close the bug on Bugzilla?

@schalkneethling schalkneethling merged commit 0ed734a into mdn:master Apr 17, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@welcome

This comment has been minimized.

welcome bot commented Apr 17, 2018

Congrats on merging your first pull request! 🎉🎉🎉

@MatsPalmgren

This comment has been minimized.

Contributor

MatsPalmgren commented Apr 17, 2018

Thanks for merging; I've closed the Bugzilla bug report now.

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