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

Patch for ISO and Map display #45

Merged
merged 6 commits into from
Feb 9, 2018
Merged

Patch for ISO and Map display #45

merged 6 commits into from
Feb 9, 2018

Conversation

johannrichard
Copy link
Contributor

Implements #43 and adds a (Static) map display on the page. Hope style and naming convention fit you.

Copy link
Owner

@mpolden mpolden left a comment

Choose a reason for hiding this comment

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

Thank you for this nice contribution!

The change looks good overall, but I had a few comments that I hope you can fix.

README.md Outdated
@@ -32,6 +32,9 @@ Country and city lookup:
$ http ifconfig.co/country
Elbonia

$ http ifconfig.co/iso
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think "iso" is self-explanatory. Can you make the path country-iso?

README.md Outdated
@@ -43,6 +46,7 @@ $ http --json ifconfig.co
{
"city": "Bornyasherk",
"country": "Elbonia",
"isoCountry": "EB",
Copy link
Owner

Choose a reason for hiding this comment

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

Field should follow same naming convention as other fields. Can you make it country_iso?

api/api.go Outdated
@@ -84,6 +85,10 @@ func (a *API) newResponse(r *http.Request) (Response, error) {
if err != nil {
a.log.Debug(err)
}
iso, err := a.oracle.LookupCountryISO(ip)
Copy link
Owner

Choose a reason for hiding this comment

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

iso -> countryISO.

api/oracle.go Outdated
if err != nil {
return "", err
}

Copy link
Owner

Choose a reason for hiding this comment

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

Please remove blank line.

index.html Outdated
@@ -63,6 +63,10 @@ <h2>Country lookup:</h2>
<pre>
$ http {{ .Host }}/country
{{ .Country }}</pre>
<h2>Country ISO lookup:</h2>
Copy link
Owner

Choose a reason for hiding this comment

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

Can you make the text "Short country code (ISO 3166-1 alpha-2) lookup"?

index.html Outdated
@@ -98,6 +103,13 @@ <h2>Testing port connectivity:</h2>
"port": 8080,
"reachable": false
}</pre>
{{ end }}
</div>
{{ if .IsLookupCityEnabled }}
Copy link
Owner

Choose a reason for hiding this comment

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

Should check that both city and country are not "Unknown".

Copy link
Contributor Author

@johannrichard johannrichard Feb 4, 2018

Choose a reason for hiding this comment

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

Not sure the way I implemented it is the "right" way in Go terms. I decided to check both for having the Country/City Lookup enabled and to check whether any of the two values returns Unknown. I didn't find another way to properly chain the checks, hope it's fine the way I did it.

index.html Outdated
</div>
{{ if .IsLookupCityEnabled }}
<div class="pure-u-1 pure-u-md-1-1">
<h2>MAP</h2>
Copy link
Owner

Choose a reason for hiding this comment

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

"MAP" -> "Map".

@mpolden mpolden merged commit c195bae into mpolden:master Feb 9, 2018
@mpolden
Copy link
Owner

mpolden commented Feb 9, 2018

Thanks!

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

Successfully merging this pull request may close these issues.

None yet

2 participants