[WIP] add an SMS install link form #3573
Conversation
| @@ -26,6 +26,15 @@ | |||
| {{#t}}Download Firefox for mobile devices.{{/t}} | |||
| </p> | |||
|
|
|||
| <form class="marketing-sms-link"> | |||
There was a problem hiding this comment.
I dig it. Are we going to show this to users we can reasonably assume to be in the US, maybe based off of locale? What happens if a non-US number is entered, will basket return an error message?
There was a problem hiding this comment.
Basket will return an error, yes. I could also add some simple validation, basically /^\d{9,10}$/, and maybe something to filter out dashes. Still looking where I would put that in the code...
There was a problem hiding this comment.
I could talk myself into several places.
- marketing-email-client->sendSms. Whether the number should/shouldn't contain dashes, dots, spaces, etc seems a bit like a detail that mostly concerns the server. Another upside is the code is shared if we ever decide to add the form somewhere else. The downside is it's far from where the input originates.
- marketing-email-snippet-ios->submit because that's close to the user input. The downside is the code wouldn't be shared if we move the form elsewhere.
- marketing-email-prefs->sendSms. Shared code, seems reasonable, but far from user input.
seanmonstar
force-pushed
the
marketing-sms
branch
3 times, most recently
from
e44c2bd
to
88e8870
Compare
|
Well hey, got the tests passing. Probably need more though... And:
|
| @@ -26,6 +26,15 @@ | |||
| {{#t}}Download Firefox for mobile devices.{{/t}} | |||
| </p> | |||
|
|
|||
| <form class="marketing-sms-link"> | |||
| <div class="input-row"> | |||
| <input type="text" id="phone-number" class="phone" /> | |||
There was a problem hiding this comment.
Ahha, the form will be presented in addition to the other links, not in lieu of. @ryanfeeley - does that match your expectations?
seanmonstar
force-pushed
the
marketing-sms
branch
from
88e8870
to
4a034a1
Compare
seanmonstar
force-pushed
the
marketing-sms
branch
from
4a034a1
to
c648ab2
Compare
|
Hey @seanmonstar - are you still working on this feature? If so, ignore the rest of this message. If not, mind shutting this PR down and re-opening when you get back to it? |
I believe I need a test for the
view, which I'm still reading how I should test. And this is also missing showing a success or failure...Closes #3497