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

Give arbitrary elements access to the image/ratio classes #592

Merged
merged 2 commits into from
Oct 31, 2018

Conversation

rjv
Copy link
Contributor

@rjv rjv commented Mar 23, 2017

Proposed solution

I often want to make other elements besides images sized to a particular ratio. With this solution, rather than the ratio classes only responding to .image > img, I propose adding a .image > .has-ratio class which can be applied to any element (e.g. <iframe>, <video>, etc).

A practical example would be the following:

<figure class="image is-16by9">
  <iframe class="has-ratio" width="640" height="360" src="https://www.youtube.com/embed/YE7VzlLtp-4?showinfo=0" frameborder="0" allowfullscreen></iframe>
</figure>

Tradeoffs

  • It relies on the parent element having the .image class, which could be misleading depending on what the child element is.
  • We could specify more element selectors (e.g. video, iframe) rather than adding another class, but that would limit its utility and increase the CSS size much more.

Testing Done

I implemented the example described in the proposal to verify it effects all elements with the .has-ratio class the same way it effects img.

@jgthms jgthms self-assigned this Mar 23, 2017
@jgthms
Copy link
Owner

jgthms commented Jun 27, 2017

Hey @rjv thanks for your contribution! Can you simply make a PR with the .sass file only? I'll merge it right away.

@ajinkyapisal
Copy link

@rjv Do you plan to make suggested changes? I'll be happy to take it further if you are busy

@rjv
Copy link
Contributor Author

rjv commented May 26, 2018

Updated the PR to only include the change to the image.sass file. Thanks @ajinkyapisal for prompting me about this.

@rjv
Copy link
Contributor Author

rjv commented Jun 28, 2018

@jgthms Could you review this again? I believe we can take the "needs work" label off.

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.

3 participants