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

Add docs explaining how to test the client on a real mobile device #330

Merged
merged 3 commits into from
Apr 3, 2017

Conversation

robertknight
Copy link
Member

While testing #327, #328 and #329 I noticed that we do not have documentation explaining the steps required to test the client on an actual device, which are a little different than testing an ordinary web page.

This PR explains the steps required to test using a device on the same network as the dev system by configuring the client/service to use URLs that reference the host name of the dev system rather than localhost. There are possibly other ways for specific device/OS combinations, but this approach is generic.

Testing the client on a mobile device currently requires several steps
which will be non-obvious to new developers. Add documentation to guide
users through the process.
@robertknight robertknight requested a review from seanh March 31, 2017 09:57
@codecov-io
Copy link

codecov-io commented Mar 31, 2017

Codecov Report

Merging #330 into master will decrease coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #330      +/-   ##
==========================================
- Coverage   76.56%   76.54%   -0.03%     
==========================================
  Files         120      121       +1     
  Lines        5949     6079     +130     
  Branches      964      993      +29     
==========================================
+ Hits         4555     4653      +98     
- Misses       1394     1426      +32
Impacted Files Coverage Δ
src/sidebar/components/annotation-share-dialog.js 78.12% <0%> (-21.88%) ⬇️
src/annotator/sidebar.coffee 76.41% <0%> (-3.34%) ⬇️
src/sidebar/virtual-thread-list.js 95.53% <0%> (-0.18%) ⬇️
src/annotator/annotation-sync.coffee 75.6% <0%> (ø)
src/sidebar/store.js 91.22% <0%> (+0.15%) ⬆️
src/sidebar/components/thread-list.js 70.66% <0%> (+2.8%) ⬆️

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 516420f...071811d. Read the comment docs.

@seanh seanh self-assigned this Mar 31, 2017
Copy link
Contributor

@seanh seanh left a comment

Choose a reason for hiding this comment

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

Thanks Rob, it's great to have docs for this. I think these will save people a lot of time, not to mention encourage more testing on mobile devices.

I realise when reading these docs that it's actually really tricky to get everything setup to test on a mobile device. All the different URLs in env vars that need to be set correctly. All the ways in which it's possible for the sidebar to simply fail to appear (with no access to an error message, on a mobile device), or to appear to work fine but to actually be the production sidebar, or to be loading assets from production.

So it's great to have this set out in docs.

I've made a couple of small requests which I hope might clarify things

@@ -0,0 +1,34 @@
# Mobile Development

## Testing the client on a mobile device
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be Testing the Client on a Mobile Device (I've used Title Case everywhere in the client docs)

If you have made changes to the client that could affect the mobile experience, it is a good idea to test them on a real device. Such changes should ideally be tested with at least current versions of iOS Safari and Chrome for Android.

1. Make sure your development system and mobile device are on the same local network.
1. Get the IP address or host name of your development system (`$HOSTNAME` in the steps below). You can do this using the `hostname` terminal command on Mac/Linux.
Copy link
Contributor

Choose a reason for hiding this comment

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

I've a couple of edit to suggest to this "Get the IP address or host name..." step:

  1. Move the "Configure the “h” service to allow incoming connections..." step below to before this step. That way, by the time they get to this step, visiting http://$HOSTNAME:5000 on the mobile device should show the h front page, so they can test whether they've set hostname correct.

  2. Instead of "Get the IP address or hostname..." this step should be "Set the $HOSTNAME environment variable to the IP address or hostname..." You use $HOSTNAME, which looks like an env var, in the steps below but it doesn't clearly tell them to set this env var, easy to miss.

  3. Edit this step to give some helpful and yet non-commital advice on getting the hostname or IP. This is what I came up with:

1. Set the `HOSTNAME` environment variable to the host name or IP address of
   your development machine on your local network, for example:

       $ export HOSTNAME=robs_laptop
   
   We'll use this environment variable in the steps below. It needs to be the
   hostname or IP address that the web browser running on your mobile device
   can use to browse to your development machine.

   **Tip**: The ``hostname`` command will tell you the hostname of your machine:

       $ hostname
       robs_laptop

   Browsing to <http://robs_laptop:5000> on your mobile device should open the
   h front page, if an h development server is running on port 5000 on
   robs_laptop. If this doesn't work (replace `robs_laptop` with your own host
   name!) then:

   1. Try appending `.home` or `.local` to the host name:
      <http://robs_laptop.local:5000>, or

   2. Find the local network IP address of your development machine and use
      that instead: <http://192.168.1.16:5000>

   Set the `HOSTNAME` environment variable to whatever works for you.

   **Note**: The `HOSTNAME` environment variable should be set to just the
   hostname or IP address part, without the `http://` or the `:5000`.
   `robs_laptop`, not `http://robs_laptop:5000` or `robs_laptop:5000`.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank-you for the suggestion but I think this is way too verbose and distracts from the Hypothesis-specific issues. I've added a sentence noting that if the hostname does not have a .home or .local suffix, the user may need to add it, and if that doesn't work (for many reasons), they should just use the IP address.

Regarding $HOSTNAME, I take your point but I think setting an env var might give the misleading impression that the commands make use of this env var name internally, as opposed to it just being a placeholder in the docs. I've left it for now and will wait for feedback from people trying to actually follow these docs.

Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW HOSTNAME is automatically set on most Linux systems. Asking people to set it themselves is just asking for trouble.

Copy link
Contributor

Choose a reason for hiding this comment

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

Regarding $HOSTNAME, I take your point but I think setting an env var might give the misleading impression that the commands make use of this env var name internally, as opposed to it just being a placeholder in the docs.

That's a good point. I'd suggest maybe using a style other than $HOSTNAME, maybe <HOSTNAME>? We could then use the same style throughout for docs placeholders.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, I like your replacement Tip, by considering hostname as a docs placeholder you've been able to keep it much shorter. I do think it's good to tell them how to test, at this step, whether they have the hostname right (by visiting h from their phone), as things stand the hostname could be wrong but they wouldn't know it until step 6 (by which time the problem could be a number of other things as well). I'd have left in the http://robs_laptop..local:5000 example as well, examples always make instructions clearer and easier to read, even a simple instruction to append something. But +1 on the conciseness though. (And I'm happy to approve it as-is, too)


gulp watch
```
When `gulp watch` runs, it will print out the URLs used for the "h" service and client assets. These should include `$HOSTNAME` instead of `localhost`.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a good troubleshooting tip (as opposed to part of the actual steps you need to follow) so I would change this to a **Tip:**. Since this is markdown we can't use a proper rst .. tip:: box but we can at least use a paragraph starting with **Tip:**.

gulp watch
```
When `gulp watch` runs, it will print out the URLs used for the "h" service and client assets. These should include `$HOSTNAME` instead of `localhost`.
1. On your mobile device, go to a page which has the client embedded such as `http://$HOSTNAME:3000` or `http://$HOSTNAME:5000/docs/help`.
Copy link
Contributor

Choose a reason for hiding this comment

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

I would use robs_laptop here and say "replace robs_laptop with the value that you set your $HOSTNAME environment variable on your development system". Because they can't actually type $HOSTNAME into their browser URL (whereas every other step where $HOSTNAME is used they can actually type in the env var ref directly)

Copy link
Member Author

Choose a reason for hiding this comment

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

Again, I opted not to do this because I think the user should be able to figure out that $HOSTNAME is a placeholder. If I get feedback from people trying to follow these steps that this isn't clear then I'll revise it.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 I think this all comes down to me being distracted by $HOSTNAME looking like an env var, but it is indeed much simpler if it's just a docs placeholder.

I think the concept of a docs placeholder is clearly very useful to have and if we have a clear and consistent style that we use for them, the docs will be easy to follow


make dev
```
1. Configure the client to load assets from this hostname and start the dev
Copy link
Contributor

Choose a reason for hiding this comment

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

To do this they'll need to open a second terminal emulator window (which I'm sure people will figure out, but I like to call this out explicitly as it makes clearer), and make sure $HOSTNAME is set in that shell too (which I think people will likely forget). Might be worth adding that to the code block below:

  1. In a second shell, configure the client to load assets from this hostname and start the dev server:
   # In the "client" repository

   # Set HOSTNAME to the same value that you set it to in the first shell above.
   export HOSTNAME=robs_laptop

   # Set URL which sidebar app ("app.html") is loaded from
   export H_SERVICE_URL=http://$HOSTNAME:5000

   ...

Copy link
Member Author

Choose a reason for hiding this comment

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

I opted not to do this for the reason mentioned above, I don't want to give the impression that HOSTNAME is anything other than a placeholder in the docs.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, and if it's just used as a docs placeholder making this unnecessary, then that makes the instructions simpler too 👍

 * Add note about possible need to add `.local` or `.home` suffix or use
   IP address instead of hostname.
 * Use title case for heading.
@robertknight robertknight force-pushed the mobile-dev-setup-docs branch from b563f2e to 3951aa2 Compare April 3, 2017 10:46
@robertknight
Copy link
Member Author

I've updated this branch. As I noted in the comments, I explicitly chose not to get the user to export a $HOSTNAME env var because I think that could give the wrong impression about the naming having some meaning to the other commands - which it doesn't currently. I'm assuming that users are able to understand it is a placeholder.

If we get feedback from users trying to follow it that they got confused then I'm happy to reconsider.

@seanh
Copy link
Contributor

seanh commented Apr 3, 2017

I have to say I think it's up to us to determine whether or not our docs are clear rather than waiting for user feedback. Users for the most part aren't going to give us feedback on our docs, and for these docs in particular the intended user audience is us (and also I reviewed these docs by following them from scratch and seeing what confused me). If we do get some user feedback we should consider it of course.

That said, I think you're absolutely right that treating hostname as a docs placeholder is a much better approach here that results in clearer and simpler docs. I didn't think of it, I just jumped straight to "this looks like an env var, so we should make it one".

I'm off to bed now so I'll approve this and let you decide, but I think I'd still like to see $HOSTNAME written as <HOSTNAME> (or whatever style you prefer for docs placeholders) so that it doesn't look like an env var. There are no other env vars on this page though (at least, not any referenced using $ENVVAR) so the confusion isn't too bad. But I think it'd be great to have a specific typographic style for docs placeholders and encourage their use liberally throughout the docs. I'll leave it up to you.

Use angle brackets to indicate that HOSTNAME is a placeholder rather
than a dollar-sign to avoid confusion with the `$HOSTNAME` env var.
@robertknight
Copy link
Member Author

I'm off to bed now so I'll approve this and let you decide, but I think I'd still like to see $HOSTNAME written as <HOSTNAME>

This sounds sensible to me. Done.

@robertknight robertknight merged commit d7e75fd into master Apr 3, 2017
@robertknight robertknight deleted the mobile-dev-setup-docs branch April 3, 2017 16:10
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.

4 participants