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.

Sign up for GitHub

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

Jump to bottom

Implementation for global pincodes #34

Closed
wants to merge 2 commits into from
Closed

Implementation for global pincodes #34

wants to merge 2 commits into from

Conversation

abhushanaj
Copy link
Contributor

@abhushanaj abhushanaj commented Mar 13, 2020 •
edited
Loading

fixes #2

@plxity here's what the PR offers:

As discussed since the only way to achieve this would be via offline data set, I have implemented the same version in React demonstrated here.

As discussed right now the data set only includes for India and US zip codes and so it'll work only for that.

The issue about the bundle size optimization is addressed via an API as you suggested, the API not not built extremely robust it's just enough to be up and running and I have used the same to minimize bundle size.

Here's a small example for it's working:
React App

Future Improvements which can be done:

  • Improving the API
  • Improving the dataset (For India it works extremely well so it can be avoided )
  • The additional input to add states, so seperate json files can be kept instead of having on huge file

yellowwoods12
yellowwoods12 approved these changes Mar 14, 2020
View reviewed changes
Copy link
Collaborator

@yellowwoods12 yellowwoods12 left a comment

Choose a reason for hiding this comment

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

@abhu-A-J Looks good to me.

@yellowwoods12
Copy link
Collaborator

@abhu-A-J We need to remove multiple similar fields when one types a pincode. Like I can see multiple "Pune" in the city dropdown and similarly in state.

@yellowwoods12
Copy link
Collaborator

yellowwoods12 commented Mar 14, 2020 •
edited
Loading

@plxity Also lets keep this to a separate testing branch since it is a completely different development for the module. We'll also encourage other participants to work on the possible improvements to this PR.

@abhushanaj
Copy link
Contributor Author

abhushanaj commented Mar 14, 2020 •
edited
Loading

@abhu-A-J We need to remove multiple similar fields when one types a pincode. Like I can see multiple "Pune" in the city dropdown and similarly in state.

@yellowwoods12 The repetition is due to the dataset itself. It's because the dataset has similar fields, if we use a dataset that has no repetition then that issue won't be present. It's completely dependent on the offline data used.

@plxity
Copy link
Collaborator

plxity commented Mar 14, 2020 •
edited
Loading

@plxity Also let's keep this to a separate testing branch since it is a completely different development for the module. We'll also encourage other participants to work on the possible improvements to this PR.

Yes, definitely.

@plxity
Copy link
Collaborator

plxity commented Mar 14, 2020

@abhu-A-J You can remove duplicate fields in this too. Please make those changes then we are going to merge this PR.

@abhushanaj
Copy link
Contributor Author

abhushanaj commented Mar 14, 2020 •
edited
Loading

@abhu-A-J You can remove duplicate fields in this too. Please make those changes then we are going to merge this PR.

@plxity The duplicate data fields are present because the dataset itself has duplicate field. If we can get the data set with non-repetitive fields, this issue won't be present. It's not a side effect from code. You can see this here the data coming back from API endpoint created.

@yellowwoods12
Copy link
Collaborator

@abhu-A-J Yes we get that. What we are suggesting here is to filter out duplicate entries after you've fetched them from the API.

@abhushanaj
Copy link
Contributor Author

abhushanaj commented Mar 14, 2020 •
edited
Loading

@yellowwoods12 @plxity I have removed the problem for repetitive data. Here is the working now.
Hope it's okay now??

React App

yellowwoods12
yellowwoods12 approved these changes Mar 16, 2020
View reviewed changes
Copy link
Collaborator

@yellowwoods12 yellowwoods12 left a comment

Choose a reason for hiding this comment

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

@abhu-A-J Good Work!

@plxity
Copy link
Collaborator

plxity commented Mar 17, 2020

@abhu-A-J Can you raise this PR to the development branch?

@abhushanaj abhushanaj mentioned this pull request Mar 18, 2020
Merged
@abhushanaj
Copy link
Contributor Author

abhushanaj commented Mar 18, 2020 •
edited
Loading

@abhu-A-J Can you raise this PR to the development branch?

Sure, I'll do the same, so I'm closing this PR and make a new PR to development branch.

@abhushanaj abhushanaj closed this Mar 18, 2020
@abhushanaj abhushanaj deleted the issue_fix_2 branch March 19, 2020 08:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yellowwoods12 yellowwoods12 yellowwoods12 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

As of now this module works only for Indian Pinocdes. Implement it for global pincodes.
3 participants
@abhushanaj @yellowwoods12 @plxity