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

Store country code in Vessel Information page #336

Merged
merged 2 commits into from
Oct 2, 2020

Conversation

nugmanoff
Copy link
Contributor

Hey there! I am happy to contribute to your repository as part of Hacktoberfest 2020

This PR Closes #332

Some rationale behind my changes:

• I renamed image variable to code in CountryPickerData because it is quite because previous name is quite ambiguous and misleading per se. So now variable code basically describes what kind of value it holds (short country code). image variable was used only once when magically retrieving country emoji, and even in that context it makes more sense to write function like flag(for countryCode: String)
• This change basically enabled me to retrieve $0.code directly and assign it to vessel.nationality in binding closure without introducing ambiguity (it'd be less obvious if it was self.vessel.nationality = $0.image, but it'd have worked the same way it does now).
• Also I fixed name of the CountryPickerView.swift file.

@Sheeri
Copy link
Collaborator

Sheeri commented Oct 1, 2020

Hi @nugmanoff - Thanks so much for this PR! We appreciate that you gave your rationale for the things we might say "why did they do that?"

Our team will fully review this by this time on Monday (three business days). Until then, please feel free to work on another issue.

@am-MongoDB am-MongoDB added this to In progress in O-FISH iOS via automation Oct 1, 2020
@o-fish-wildaid
Copy link
Collaborator

Review PRs

@am-MongoDB am-MongoDB self-requested a review October 1, 2020 14:00
Copy link
Collaborator

@am-MongoDB am-MongoDB left a comment

Choose a reason for hiding this comment

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

What's in the PR is good, but there's one another change needed.

When you view an existing report, the country should be mapped back from the (stored) code to the full country name – this is what the view currently looks like, but GB should actually be displayed as United Kingdom
image

O-FISH iOS automation moved this from In progress to Review in progress Oct 1, 2020
@nugmanoff
Copy link
Contributor Author

@am-MongoDB thanks for pointing that one out, I missed that! Can you please check the updated revision?

Copy link
Collaborator

@am-MongoDB am-MongoDB left a comment

Choose a reason for hiding this comment

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

I tested the latest and the report view seems correct, but it looks like 140 older commits have been dragged into this PR
image

Could you please try to clean up the PR so that it only contains your changes?

Thanks, Andrew.

@nugmanoff nugmanoff force-pushed the store-country-code branch 3 times, most recently from 2dfa5d1 to fb9950b Compare October 1, 2020 20:20
@am-MongoDB
Copy link
Collaborator

am-MongoDB commented Oct 1, 2020

@nugmanoff I'm not a git-guru and so I always keep it simple.

If you can't get your current fork into line, then:
[ ] Take a copy of the files you've changed
[ ] Create a clean fork
[ ] Reapply your changes
[ ] Create a new PR, include fixes #332 and a reference your original PR in the description

Andrew.

@nugmanoff
Copy link
Contributor Author

@am-MongoDB thanks! but I think I've already fixed the issues

@am-MongoDB am-MongoDB merged commit 7217011 into WildAid:main Oct 2, 2020
O-FISH iOS automation moved this from Review in progress to Scoped for demo Oct 2, 2020
@am-MongoDB
Copy link
Collaborator

Thanks @nugmanoff PR merged - thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
O-FISH iOS
  
Scoped for demo
Development

Successfully merging this pull request may close these issues.

For Flag State on Vessel Information Store Country Code
4 participants