devex icon indicating copy to clipboard operation
devex copied to clipboard
verified publisher icon ExchangeBC

Suggestions to improve documentation

Open chadfawcett opened this issue 7 years ago • 2 comments

When setting up the repo to work on #552 I ran into several things that were unclear or seemed out of date with the README. I thought I would try to make note of the things that were unclear in order to shed some light on how the project looks as a newcomer. I don't have the time in the immediate future to make a PR updating the docs, but may be able to in the future. I think this would be a good first issue for someone to run through the docs from scratch and fix anything that's out of date.

Note: I am running within docker-machine, so please excuse any issues that are related to the fact that I was not running directly on the host.

  • Is there a reason for the inconsistency between the container and host ports for the app? (ie 3030:3000) It's not a huge deal, but the console outputs that the server is running on port 3000, so I first try to access it there. I had read the README through once already so I thought I remembered it being 3030 which prompted me to look at the README again, as well as the dev.sh file. If someone was to miss that in the README, they could get led on a wild goose chase trying to find why they couldn't access it on port 3000.
    • Suggestion: change both ports to be the same, so the port logged out in the console is the ones used to access the app from the browser.
  • The bash scripts need to be called using their path (ie ./setup.sh, ~/devex/setup.sh, $(pwd)/setup.sh, etc)
    • Suggestion: Update the README to ./setup.sh and ./dev.sh
  • The dev container was already using Node 8, and nvm wasn't installed as the README suggests.
    • Suggestion: Remove this section from the README
  • None of the test scripts worked, and neither of the locations for tests exist (app/tests/ and public/modules/*/tests/)
    • Suggestion: It looks like there have been some improvements/changes to the tests. README should be updated to show how to run new tests.
  • The commit message guidelines should be referenced in the README. I was lucky to only have a single commit in my PR, so it was easy to update the commit. It'd be a pain to have to go back and edit the messages for multiple commits. I looked at previous PRs, and it didn't look like anyone was using this commit convention, so maybe it is no longer a requirement.
    • Suggestion: The PR template shouldn't be the first place to enforce commit message conventions. Ideally I would be aware of this before making my first commit. Alternatively, if this isn't a convention actually in use, the PR template should be updated.
  • npm run dev with watch mode wasn't updating when changing files. Granted, this command isn't documented, so it may or may not be recommended to use. I just didn't want to have to stop and start the server each time I made a change. So I went looking for a dev/watch mode. I tried forcing the dev restart with rs, but had mixed results. Not sure if there is a caching issue, but I usually had to completely stop the server and start it again before my changes took place.
    • Suggestion: Update or remove the dev/watch mode
  • There was nothing showing how to login using the usernames and passwords seeded into the DB. I had to search the codebase for alternate login routes (/authentication/signinadmin) in order to login. I initially tried signing in with GitHub but got a redirect-uri-mismatch error. The fact that there are GitHub secrets committed into the repo leads me to believe these are for development, and the callback uri is probably set to http://localhost:3030. However, since I am running inside docker-machine I wasn't actually accessing the app on localhost so my uri didn't match. Finding the admin login page was quicker than me creating my own GitHub app for testing, so I haven't confirmed this.
    • Suggestion: Document the intended login method in development. If GitHub is the intended method, the seeded logins shouldn't be presented. At least mentioning that the GitHub login is supposed to work, I may have gone deeper into getting that to work.

May I also suggest adding a CODE_OF_CONDUCT to formally outline the expected behaviour of contributors and participants?

I think that is everything I came across. Again, please excuse anything related to the fact that I'm running within docker-machine, but I feel at least some of these suggestions would help future contributors.

chadfawcett avatar Jan 22 '19 20:01 chadfawcett

Thanks for the suggestions @chadfawcett . The documentation is indeed out of date, and these suggestions will help us prioritize. It seems like ones that would be most helpful from that list would be setting up the Github redirects and the paths for the bash scripts. Is that a good assumption?

Regarding the dev/watch mode, using the 'dev' script should launch the application with client reloading using livereload over a websocket on port 35729, and gulp-watch on the server side. I have noticed recently that the server won't automatically restart like it should, so I will look at addressing that. But you should be seeing your changes automatically applied in the client (and an automatic page refresh), and if you are using 'rs' to restart the server, your changes should be reflected after that as well. It may be docker-machine is making things behave a little differently, so I'll check this out too.

sutherlanda avatar Jan 22 '19 21:01 sutherlanda

@sutherlanda docker-machine is becoming less and less common (at least from what I've seen) since they added support running directly on Mac OS. So if the majority of newcomers aren't running in docker-machine and the GitHub auth works as expected when redirecting from http://localhost:3030, then I'm not sure what you'd want to do here. Unless you've thought of a different solution, I think the only way to support docker-machine and native host easily would be to use a custom host like http://docker:3030, but this just adds an extra step and complexity to the setup. Unless GitHub allows for multiple referral uri's... you could do localhost and a custom host. Really, I'd just say if the GitHub auth works as expected when not using docker-machine I would just document the fact that the signinadmin is available if someone wants to develop without authenticating with GitHub.

I would think using the proper path for the bash scripts is fairly intuitive for anyone familiar with command line. However, it's a simple enough change to make it easier for beginners.

That was my assumption on how the dev/watch mode worked, but the server didn't restart (which makes sense since I didn't make any server side changes) nor did the browser refresh. When running npm start I get an error in the console saying the live-reload fails to connect, and don't get an error when running npm run dev. Either that means it's connecting properly in dev, or things are backwards and the front-end is trying to connect to the live-reload when in start and not dev. Either way, I just confirmed, if I make a change to the front end, even a view, the browser does not reload automatically.

chadfawcett avatar Jan 22 '19 23:01 chadfawcett