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

Add new spacer component #21

Merged
merged 4 commits into from Mar 28, 2016
Merged

Conversation

acezard
Copy link
Contributor

@acezard acezard commented Mar 25, 2016

Suggesting a component used for accurate vertical spacing.

It would be used that way : <spacer size="10"></spacer> and generates a battle-tested
<table class="spacer"><tbody><tr><td height="10px" style="font-size:10px;mso-line-height-rule: exactly;line-height:10px;">&nbsp;</td></tr></tbody></table>

I feel this kind of component can be helpful in the framework because it allows an easy way of managing padding/margins without ever using them (usually unreliable on outlook). For instance it's very nice to make separations between blocks (even with different background colors), or even between elements in a single column.

I will make a pr to the foundation for emails repo because a css rule is also needed (100% width)

@gakimball
Copy link
Contributor

If there's CSS that needs to be added to Foundation for Emails, then can we add all the CSS you've got in the style attribute? That way we keep Inky's HTML output cleaner.

@acezard
Copy link
Contributor Author

acezard commented Mar 28, 2016

It's possible. The only issue is that the user sets himself the height of the spacer element with the size attribute directly in the element attribute. Instead, I think it would be possible to create defaut css classes like "space-md", "space-lg", or allow the user to create custom classes if he knows what properties to target. I was under the impression that it might be less user-friendly, what are your thoughts ?

@gakimball
Copy link
Contributor

@acezard Oh, I'm sorry, I was totally blind to that :) Makes total sense.

Need to do a bit more internal review with the team and then we can bring this in. Thanks again!

@rafibomb
Copy link
Member

We like the way you have it @acezard ! I'll be testing it this week to bring it in. Thanks!

@gakimball
Copy link
Contributor

@acezard Can you put together a unit test for this new component? We'll need that before we can merge. It would go under test/components.js.

@acezard
Copy link
Contributor Author

acezard commented Mar 28, 2016

I added an unit test and also removed an unnecessary inline css property (I put it directly into the css instead. Not sure if that rule is really necessary though but it was suggested to me on the initial issue foundation/foundation-emails#289).

@gakimball
Copy link
Contributor

Awesome, thank you!

@gakimball gakimball merged commit 63ae6b9 into foundation:master Mar 28, 2016
@acezard acezard deleted the feature/spacer branch March 28, 2016 19:18
@rafibomb
Copy link
Member

@acezard Shoot us an email with your address for some Yeti stickers! foundation at zurb .com

@rafibomb
Copy link
Member

@acezard Just wanted to make sure I didn't miss your email, did you have a chance to send it?

@acezard
Copy link
Contributor Author

acezard commented Apr 21, 2016

@rafibomb oh, yea I did send it about the time you posted the previous message ! maybe I made a mistake ? my email adress is my github username @gmail.com

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants