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

fix: use display table for the thumnails in the docs #30

Merged
merged 2 commits into from
Mar 9, 2018
Merged

fix: use display table for the thumnails in the docs #30

merged 2 commits into from
Mar 9, 2018

Conversation

DanielRuf
Copy link
Contributor

@DanielRuf DanielRuf commented Mar 9, 2018

The styles of the thumbnails in the docs use display: block
with auto margins on both sides for exemplary centering them.

But this does not work well with display: block on an a tag.
display: table text-center on the parent works much better in this example which is just for the docs and demonstrating the thumbnails.

Closes foundation/foundation-sites#11029

Copy link
Contributor

@ncoden ncoden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is no bug in Foundation Sites, the styles are only in the docs.
-- foundation/foundation-sites#11029 (comment)

It's hard to believe as this change the style and behavior of the thumbnail itself.

display: block should not have been applied in the first place. I think that a solution like text-align: center on the parent (the column) is far more cleaner and less conducive to unwanted side effects.

@DanielRuf
Copy link
Contributor Author

It's hard to believe as this change the style and behavior of the thumbnail itself.

Just the doc styles are problematic.

text-align: center on the parent (the column) is far more cleaner and less conducive to unwanted side effects.

This will not work here ;-)

@ncoden
Copy link
Contributor

ncoden commented Mar 9, 2018

This will not work here ;-)

I mean, by removing display: block and auto-margin. We want to center an inline-block element, the proper way to do that is with text-align: center on the parent ;)

@DanielRuf
Copy link
Contributor Author

Would be a cleaner approach, right. I'll change it.

@DanielRuf
Copy link
Contributor Author

But wait, the codepen does not center them https://codepen.io/IamManchanda/pen/EmLexY?editors=1100

So we should be consistent. To center or not to center?

@ncoden
Copy link
Contributor

ncoden commented Mar 9, 2018

I vote for "to center" ;)

@DanielRuf
Copy link
Contributor Author

And the example uses the float grid (not the new xy grid) which has no center class or did I oversee something? small-centered does not work here.

@ncoden
Copy link
Contributor

ncoden commented Mar 9, 2018

text-center should be enouth there, or you can create a dedicated class.

@DanielRuf DanielRuf closed this Mar 9, 2018
@DanielRuf DanielRuf deleted the fix/thumbnail-display-table branch March 9, 2018 21:46
@DanielRuf DanielRuf restored the fix/thumbnail-display-table branch March 9, 2018 21:47
@DanielRuf DanielRuf reopened this Mar 9, 2018
@ncoden
Copy link
Contributor

ncoden commented Mar 9, 2018

What was that ? 😄

@DanielRuf
Copy link
Contributor Author

What was that ?

? =/

I accidentally deleted it, nothing more. Sorry for the confusion. The GitHub UI is crap.

Copy link
Contributor

@ncoden ncoden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Thanks. You can update foundation-docs in foundation/foundation-sites#11032

@ncoden ncoden merged commit 60e2428 into foundation:master Mar 9, 2018
@DanielRuf DanielRuf deleted the fix/thumbnail-display-table branch March 9, 2018 22:06
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.

2 participants