Add geoip javascript client guide #87

kevcenteno · 2021-01-22T23:47:04Z

No description provided.

github-actions · 2021-01-22T23:49:08Z

Jest Coverage

File	% Stmts	% Branch	% Funcs	% Lines	Uncovered Line #s
All files	100	99.14	100	100
_gatsby/gatsby-config/remark	100	92.31	100	100
_ sectionize.ts	100	92.31	100	100	35
_gatsby/gatsby-node/pages/createApiSpecPages/utils	100	100	100	100
_ addCompiledExamples.ts	100	100	100	100
_ addLineNumbers.ts	100	100	100	100
_ getPrimitive.ts	100	100	100	100
_ normalizeArray.ts	100	100	100	100
_ normalizeObject.ts	100	100	100	100
_ openapi.fixture.ts	100	100	100	100
_ primitives.ts	100	100	100	100
_ sampleFromSchema.ts	100	100	100	100
_ sortKeys.ts	100	100	100	100
_ transformDocument.ts	100	100	100	100
_types	100	100	100	100
_ Item.ts	100	100	100	100
_utils	100	100	100	100
_ openapi.ts	100	100	100	100
_ pagination.ts	100	100	100	100
Done in 6.74s.

github-actions · 2021-01-23T00:11:11Z

Visit the preview URL for this PR (updated for commit c68a015):

https://dev-maxmind-com--pr87-geoip-js-ns65qotw.web.app

_{(expires Mon, 01 Feb 2021 21:42:21 GMT)}

_{🔥 via Firebase Hosting GitHub Action 🌎}

oalders

Very nice work! 📝 Just a few comments to look at.

oalders · 2021-01-25T18:24:52Z

content/geoip2/web-services/using-the-javascript-client.mdx

+convenient, there are some caveats with its usage as some browser settings and
+add-ons (such as ad blockers) may prevent the GeoIP2 JavaScript API from
+successfully calling the web services, as well as unexpected usage spikes that
+you may want to monitor.


This is a pretty long sentence. I think it can be broken up for clarity. Particularly it's not 100% clear how "as well as unexpected usage spikes that you may want to monitor." fits in. I think maybe it means:

While it is convenient, there are some caveats with its usage. Some browser settings and
add-ons (such as ad blockers) may prevent the GeoIP2 JavaScript API from
successfully calling the web services. Additionally, there may be unexpected usage spikes which
you may want to monitor.

Probably also worth noting why usage might spike.

oalders · 2021-01-25T18:26:31Z

content/geoip2/web-services/using-the-javascript-client.mdx

+In order to use this service, the following JavaScript must be included in your
+page.
+
+**Do not download this JavaScript file and load it from your server(s)!**


It's clear to me why this shouldn't be done, but maybe a note to clarify what the consequences are.

oalders · 2021-01-25T18:28:05Z

content/geoip2/web-services/using-the-javascript-client.mdx

+
+| Option                                         | Description                                                                                                                     |
+| ---------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------- |
+| `geoip2.country(onSuccess, onError, options)`  | Calls the GeoIP2 Precision: Country endpoint using the routable IP address associated with the machine on which it is running.  |


I wonder if the product names should be quoted or emphasized here. Having that colon in the description without additional context is a bit confusing.

oalders · 2021-01-25T18:28:33Z

content/geoip2/web-services/using-the-javascript-client.mdx

+
+All of the functions take the same 3 arguments:
+
+* If successful, this function calls the `onSuccess`. The first parameter


calls the onSuccess.

It feels like a word is missing here.

oalders · 2021-01-25T18:29:49Z

content/geoip2/web-services/using-the-javascript-client.mdx

+Explorer 8, this is implemented as a non-enumerable property using
+`defineProperty`.
+
+* If there is an error, the function calls the `onError` with the error object


the function calls the onError

This also feels like it has a word missing, but maybe I'm just not getting the JS lingo. I think it needs a trailing period?

oalders · 2021-01-25T18:30:16Z

content/geoip2/web-services/using-the-javascript-client.mdx

+
+* The optional `options` parameter is an object containing flags for the
+
+service. This parameter is reserved for future use. There are no options at


Should there be a line break above?

wesrice · 2021-01-25T18:49:36Z

content/geoip2/web-services/using-the-javascript-client.mdx

+
+```json
+{
+  "city":     {


The spacing of this left curly brace is kind of odd.

wesrice · 2021-01-25T18:50:49Z

content/geoip2/web-services/using-the-javascript-client.mdx

+
+```js
+// demo.js
+var fillInPage = (function() {


Should we use two spaces to condense the indention a bit? We are using two spaces in the html block above this as well as in the json block below this.

wesrice · 2021-01-25T18:54:14Z

content/geoip2/web-services/using-the-javascript-client.mdx

+
+```json
+{
+  "city":         ...,


Not a huge deal, perhaps more of a personal preference. Generally, I'm not a fan of aligning properties like this. It makes maintaining a little harder when the widest property changes.

wesrice · 2021-01-25T18:57:09Z

content/minfraud/working-with-transaction-dispositions.mdx

-| (none)                 | 406 Not Acceptable         | Your request included an `Accept-Charset` header that is not supported. `UTF-8` is the only acceptable character set.                 |
-| (none)                 | 415 Unsupported Media Type | Your request included an `Accept` header that is not supported. The web service cannot return content of that type.                   |
-| (none)                 | 503 Service Not Available  | There is a problem with the web service server. You can try this request again later.                                                 |
+| Code                   | HTTP Status                | Error                                                                                                                 |


It seems like we are going to end up having a table very similar to this on a lot of pages, with some variance based on 400 requests. We don't have to abstract this now, but it might be worth starting to think about it.

wesrice · 2021-01-25T19:02:02Z

content/geoip2/web-services/using-the-javascript-client.mdx

+      "names": {}
+  },
+  "country":   ...,
+  "continent": ...,


General style question. For the ellipses that are property values, should we encase them in an enclosure that can identify their type?

Examples: "...", [...], {...}.

wesrice · 2021-01-25T19:15:46Z

content/geoip2/web-services/using-the-javascript-client.mdx

+var fillInPage = (function() {
+    var updateCityText = function(geoipResponse) {
+
+        /* It's possible that we won't have any names for this city.


Super nitpicky, but examples of most multiline comments in popular styleguides typically utilize a linebreak after the opening /* and a * on each additional line.

Some reasoning from Google – "subsequent lines must start with * aligned with the * on the previous line, to make comments obvious with no extra context."

wesrice · 2021-01-25T19:16:58Z

content/geoip2/web-services/using-the-javascript-client.mdx

+        updateCityText(geoipResponse);
+    };
+
+    /* If we get an error we will display an error message*/


We can change this to a // comment since it is a single line. If not, then a space needs to be added between message and the closing */.

kevcenteno · 2021-01-25T21:38:44Z

@oalders @wesrice Thanks for the feedback. I've made the suggested changes in c68a015

oalders

💯

Add javascript client page

bd90c0f

kevcenteno added 2 commits January 22, 2021 19:00

Add geoip javascript client api docs

a76f244

Fix build

4cc7851

kevcenteno force-pushed the geoip-js branch from 580f3bb to 4cc7851 Compare January 23, 2021 00:06

oalders suggested changes Jan 25, 2021

View reviewed changes

wesrice reviewed Jan 25, 2021

View reviewed changes

Address content feedback

c68a015

wesrice approved these changes Jan 25, 2021

View reviewed changes

oalders approved these changes Jan 26, 2021

View reviewed changes

kevcenteno merged commit fd77f51 into main Jan 26, 2021

kevcenteno deleted the geoip-js branch January 26, 2021 18:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add geoip javascript client guide #87

Add geoip javascript client guide #87

kevcenteno commented Jan 22, 2021

github-actions bot commented Jan 22, 2021 •

edited

Loading

github-actions bot commented Jan 23, 2021 •

edited

Loading

oalders left a comment

oalders Jan 25, 2021

oalders Jan 25, 2021

oalders Jan 25, 2021

oalders Jan 25, 2021

oalders Jan 25, 2021

oalders Jan 25, 2021

wesrice Jan 25, 2021

wesrice Jan 25, 2021

wesrice Jan 25, 2021

wesrice Jan 25, 2021

wesrice Jan 25, 2021

wesrice Jan 25, 2021

wesrice Jan 25, 2021

kevcenteno commented Jan 25, 2021

oalders left a comment


		All of the functions take the same 3 arguments:

		* If successful, this function calls the `onSuccess`. The first parameter


		* The optional `options` parameter is an object containing flags for the

		service. This parameter is reserved for future use. There are no options at

Add geoip javascript client guide #87

Add geoip javascript client guide #87

Conversation

kevcenteno commented Jan 22, 2021

github-actions bot commented Jan 22, 2021 • edited Loading

Jest Coverage

github-actions bot commented Jan 23, 2021 • edited Loading

oalders left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevcenteno commented Jan 25, 2021

oalders left a comment

Choose a reason for hiding this comment

github-actions bot commented Jan 22, 2021 •

edited

Loading

github-actions bot commented Jan 23, 2021 •

edited

Loading