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

Feature: Add separateDialCode option #123

Merged

Conversation

alexborovkov
Copy link
Contributor

No description provided.

@dcyriller dcyriller added the enhancement New feature or request label Sep 17, 2019
@dcyriller dcyriller self-requested a review September 17, 2019 12:43
Copy link
Contributor

@dcyriller dcyriller left a comment

Choose a reason for hiding this comment

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

You make this new property as an opt-in 👍.

May you add a test for this new option in the file tests/integration/components/phone-input-test.js?

Also, it would be nice to add a section in the playground to demonstrate this option that you add. You could add at the bottom of this file a h2 title: separateDialOption with the following snippet:

<h2>separateDialOption</h2>
{{#docs-demo as |demo|}}
  {{#demo.example name="phone-input-separate-dial-option.hbs"}}

    <p>{{phone-input separateDialCode=true initialCountry="us" number=separateDialNumber update=(action "updateSeparateDialOption")}}</p>

  {{/demo.example}}

  {{demo.snippet "phone-input-separate-dial-option.hbs"}}
{{/docs-demo}}

Then you would add at the top of the file: <h2>Basic Options</h2>.

Could add this two things?

@alexborovkov
Copy link
Contributor Author

@dcyriller Added

@alexborovkov
Copy link
Contributor Author

@dcyriller Hello, When are you planning to merge my PR ? I need this feature for my project

Copy link
Contributor

@dcyriller dcyriller left a comment

Choose a reason for hiding this comment

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

With the small changes regarding the test, I think we'll be good!

@alexborovkov
Copy link
Contributor Author

@dcyriller Agree. I've made fix.

Copy link
Contributor

@dcyriller dcyriller left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@dcyriller dcyriller merged commit a43aa60 into qonto:master Sep 23, 2019
@dcyriller
Copy link
Contributor

Released as v3.0.0 (I had to bump the major version due to a breaking PR not released: node 6 support drop).

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

Successfully merging this pull request may close these issues.

2 participants