Details

Reviewers

ashoat
bartek

Group Reviewers

Restricted Owners Package

(Owns No Changed Paths)

Commits

rCOMM4232185e500b: [docs] Fix keyserver port numer, add instructions for connecting from physical…

Summary

Issue: https://linear.app/comm/issue/ENG-2602/ip-port-in-dev-environmentmd

Keyserver runs on port 9000, so I fixed that information.
I called it "Expo lanucher screen" based on this expo doc: https://docs.expo.dev/development/use-development-builds/#the-launcher-screen
I moved the instructions to find the IP address since now they are needed one section up
I changed the instructions to find the IP address as discussed in the linear issue, because the GUI based ones were not working for macOS Ventura anymore. The terminal based ones are more universal.

Test Plan

Viewed rendered markdown

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

inka created this revision.Jan 12 2023, 1:49 AM

Herald added subscribers: atul, tomek, ashoat. · View Herald TranscriptJan 12 2023, 1:49 AM

Capitalize "Expo"

Match style of fake link with "Connecting to local keyserver" section

inka added inline comments.Jan 12 2023, 1:58 AM

docs/dev_environment.md
648–649 ↗	(On Diff #20853)	I can see there is a difference in "machines / machine's local IP address" inn those two sentences. The second one is copied from "Connecting to local keyserver", but I think "machines" is correct. Can anyone confirm or deny?

inka edited the summary of this revision. (Show Details)Jan 12 2023, 2:00 AM

inka added a reviewer: Restricted Owners Package.

Harbormaster completed remote builds in B15288: Diff 20850.Jan 12 2023, 2:04 AM

Harbormaster completed remote builds in B15289: Diff 20851.

Harbormaster completed remote builds in B15291: Diff 20853.Jan 12 2023, 2:08 AM

inka requested review of this revision.Jan 12 2023, 2:08 AM

Please try to match existing language. You're introducing patterns that are never used before in these docs, such as separating two paragraphs with a single line break instead of two. Please take a very close, critical look at your approach and revise it to match existing language as much as possible. Once that's done I'll take a look

docs/dev_environment.md
648 ↗	(On Diff #20853)	This sentence is too long with too many commas You're missing "the" in "the Expo launcher screen" Does the reader have any idea what "Expo" is? We probably shouldn't mention it

This revision now requires changes to proceed.Jan 12 2023, 7:45 AM

inka added inline comments.Jan 12 2023, 9:13 AM

docs/dev_environment.md
648–649 ↗	(On Diff #20853)	I was wrong on this, I'll correct my spelling. source

inka added inline comments.Jan 16 2023, 1:31 AM

docs/dev_environment.md
648 ↗	(On Diff #20853)	Assuming, that by "long" sentence you mean that it's too complex, not "long" as in too many words.

Try ti match the docs style better

inka added inline comments.Jan 16 2023, 2:19 AM

docs/dev_environment.md
648 ↗	(On Diff #20969)	From Connecting to local keyserver section
648 ↗	(On Diff #20969)	Also from Connecting to local keyserver section
648 ↗	(On Diff #20969)	Also from Connecting to local keyserver section
648 ↗	(On Diff #20969)	From https://github.com/CommE2E/comm/blob/master/docs/dev_environment.md#mariadb-1 section
650 ↗	(On Diff #20969)	From previous Connecting to local keyserver section
650 ↗	(On Diff #20969)	Also from previous Connecting to local keyserver section
650 ↗	(On Diff #20969)	From Running keyserver section

Harbormaster completed remote builds in B15378: Diff 20969.Jan 16 2023, 2:24 AM

English is looking much better!!

docs/dev_environment.md
648 ↗	(On Diff #20969)	Regarding the "If this is the first build" section, it looks good (outside of missing "the", see inline) but I think it may no longer be necessary after D6269 – @bartek, can you confirm?
656 ↗	(On Diff #20969)	This is actually incorrect – I think I gave you this idea at some point, but it would've been good if you confirmed it: keyservers run on port 3000, not 9000

This revision now requires changes to proceed.Jan 16 2023, 7:59 PM

bartek added inline comments.Jan 17 2023, 12:46 AM

docs/dev_environment.md
648 ↗	(On Diff #20969)	D6269 does affect the keyserver list in app options, it is actually unrelated in context of selecting app to run in "Expo launcher screen". However, D6257 is more related - now the IP address should be displayed directly in `yarn dev` console (expo/metro screen), right below the QR code.
656 ↗	(On Diff #20969)	It's 3000