Skip to content

Add optional 'target' attribute for <button>#42

Merged
rafibomb merged 1 commit intofoundation:masterfrom
ClementParis016:feature/button-target
May 18, 2016
Merged

Add optional 'target' attribute for <button>#42
rafibomb merged 1 commit intofoundation:masterfrom
ClementParis016:feature/button-target

Conversation

@ClementParis016
Copy link
Contributor

<button> components needs the ability to be passed a target attribute (that will be passed to the resulting <a> element), which could be set to _blank for use in the "view in browser" version of emails.

See foundation/foundation-emails#432 for discussion.

@rafibomb
Copy link
Member

Thanks for the PR! Do you know it's causing Travis to fail?

@ClementParis016
Copy link
Contributor Author

Yep, tests on characters encoding are failing. But I don't think it's related to the code I wrote because when I forked the repo and ran the tests for the first time, they were already failing.
I will double check that later in the day and let you know.

@ClementParis016
Copy link
Contributor Author

ClementParis016 commented May 14, 2016

@rafibomb indeed, the failure doesn't come from my additions since the exact same tests are also failing for master on my fork.
Could it be related to #37 in any way?

EDIT: these tests you've added at 41e9e58 never passed, but #37 is definetely addressing this issue.

@rafibomb
Copy link
Member

Sounds like it is fix-character-encoding. @kball is this something we can revert for now so the test pass?

@rafibomb rafibomb merged commit 848e773 into foundation:master May 18, 2016
@rafibomb rafibomb added this to the 2.1.1 milestone May 18, 2016
@rafibomb
Copy link
Member

Great PR BTW! From now on we'll be directing PR's for bug fixes to the develop branch and new features to the next point release (2.2.0 branch currently)

Merging this here - thanks!

@kball
Copy link
Contributor

kball commented May 18, 2016

@rafibomb yeah those tests should just be on the branch where we're working on the encoding. You can revert them in develop/master

@rafibomb
Copy link
Member

rafibomb commented Jun 8, 2016

@ClementParis016 It seems like we'll need to set this up for the menu items as well foundation/foundation-emails#465 Is that something you can help with? The method should be much the same.

Thanks!

@ClementParis016
Copy link
Contributor Author

@rafibomb Yep, it seems to be a quick one, I will submit a new PR in a few ;) (not sure if we could re-use this one ? I'm quite new to contributing on open source projects)

@ClementParis016
Copy link
Contributor Author

Done, #54

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.

3 participants