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

Improve image layout in the carousel example. #15648

Merged
merged 3 commits into from
Feb 9, 2015
Merged

Improve image layout in the carousel example. #15648

merged 3 commits into from
Feb 9, 2015

Conversation

Nikita240
Copy link
Contributor

The way the featurette images positioned themselves when the display was below the md threshold was very ugly.

Before After
Original layout Modified layout

Not only does it look better, it also does a much better job of showcasing bootstrap functionality. The new layout is a good example of the usefulness of push/pull. It also serves to educate people about .center-block, because it is instinctual for people to try to center images with text-align, but in this case that would not work since .img-responsive changes the display type on the the img to block (which the website has no mention of btw). See this guy (I made the same mistake): #14596.

I used this template in my project, and it took me a good while to figure out how to fix these two problems.

In order to see the changes, you must shrink the browser to below the md threshold.
When the screen is shrunk below the md threshold, all the featurette columns stack.
However, since the second featurette is "backwards", when stacked the second featurette
looked awkward as the image came before the heading.
People normally center images using text-align. However, the `.img-responsive`
class changes the display type to block. As a result you can no longer align the 
image with text-align. You must instead do it with margin: auto. There was no 
note about this on the web page, and no mention about setting the display to
block. Users were left on a frustrating journey to figure out why they can't center
images anymore!
@Nikita240
Copy link
Contributor Author

I decided to take the liberty to edit the web page on .img-responsive. I added a warning about text-align and the fact that display changes to block. I find this is necessary because there was no mention at all of .img-responsive changing the display type, so users were left on a frustrating journey to figure out why they can't center images anymore.

These are all the issues I could find within a 30 second search with instances of people trying to center align an .img-responsive:

@XhmikosR XhmikosR added the docs label Feb 2, 2015
@mdo mdo added this to the v3.3.4 milestone Feb 9, 2015
mdo added a commit that referenced this pull request Feb 9, 2015
Improve image layout in the carousel example.
@mdo mdo merged commit 2fad6e8 into twbs:master Feb 9, 2015
@cvrebert cvrebert mentioned this pull request Feb 9, 2015
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.

4 participants