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

passing country as prop instead of countryCode as it is not unique #403

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

appy9
Copy link
Contributor

@appy9 appy9 commented Jan 30, 2022

country code is not a unique key that can be passed, so updating the component to pass country

Issue Usecase: UK vs Jersey

 {
    id: 121,
    name: "United Kingdom",
    url_name: "gb",
    country_code: "+44",
    operational: true,
    zones: ["Europe/London"]
 },
 {
    id: 57,
    name: "Jersey",
    url_name: "je",
    country_code: "+44",
    operational: false,
    zones: ["Europe/London"]
 }

https://deploy-preview-403--pensive-johnson-e2d194.netlify.app/?path=/story/components-phonenumberinput--material

@netlify
Copy link

netlify bot commented Jan 30, 2022

✔️ Deploy Preview for pensive-johnson-e2d194 ready!

🔨 Explore the source changes: 74506c0

🔍 Inspect the deploy log: https://app.netlify.com/sites/pensive-johnson-e2d194/deploys/61f6325779334700078a5de1

😎 Browse the preview: https://deploy-preview-403--pensive-johnson-e2d194.netlify.app

@appy9 appy9 requested review from pranjal-jain and zhirzh January 30, 2022 06:39
@codecov
Copy link

codecov bot commented Jan 30, 2022

Codecov Report

Merging #403 (74506c0) into master (6662ecf) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #403   +/-   ##
=======================================
  Coverage   83.25%   83.25%           
=======================================
  Files          84       84           
  Lines        1362     1362           
  Branches      270      263    -7     
=======================================
  Hits         1134     1134           
- Misses        193      194    +1     
+ Partials       35       34    -1     
Impacted Files Coverage Δ
...ges/pebble-web/src/components/PhoneNumberInput.tsx 95.00% <100.00%> (ø)
packages/pebble-web/src/components/Toast.tsx 91.66% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6662ecf...74506c0. Read the comment docs.

Copy link
Member

@zhirzh zhirzh left a comment

Choose a reason for hiding this comment

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

this change wont be backwards compatible and require massive rewrites in consumer code

@appy9
Copy link
Contributor Author

appy9 commented Jan 30, 2022

this change wont be backwards compatible and require massive rewrites in consumer code

without this change we are not able to identify UK numbers from Jersey. Also, we can't correctly validate UK numbers because Jersey gets selected. The original component is actually wrong in returning just the country code.

@zhirzh zhirzh marked this pull request as draft February 16, 2023 08:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants