-
Notifications
You must be signed in to change notification settings - Fork 21
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
Updated & Restructured Dev Docs, Updated Footer and Sidebar #352
base: master
Are you sure you want to change the base?
Conversation
My two cents as a retiree - might be better to make this two separate pages, i.e.
Ngl the docs are a bit of a mess, so feel free to revamp them! |
Sounds good! With a bit of a reorganization now...
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll leave a more thorough review after I head off to Seattle (if you'd like), but my quick note is that I think the current setup is still a bit confusing: all the information about Jekyll, WSL, bundle, etc. doesn't need to be in "dev setup" since it's only related to Jekyll and the Ruby stack.
If I could redo the structure (wink wink, nudge nudge) I'm thinking:
- developer setup -> just how to use git, etc.
- website setup -> ruby, bundle, git review, wsl stuff
- website first-steps -> this is the adding yourself to team page
- node setup -> what you have now, but under "node" may make more sense. also, you may want to explain how to install node and/or nvm
Thanks for the feedback @mattxwang! I think that we can take out |
Restructured the pages more by combining static website set-up and first steps, moving all the git stuff into |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Think this is looking good. Most of these comments are nitpicky, and I would also suggest requesting review from someone on the teach la dev team!
Also, a generic comment that has too many spotchecks - I would strongly avoid using "here" as link text, since it's not very accessible. Try rewording the language so it's more clear what the link is going towards!
title: "Project Setup: Extra Installations" | ||
sidebar-title: "Project Setup" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should explicitly mention Node somewhere, since not all of our projects are in node (i.e., if someone starts on the website, it's a project!).
|
||
Once your pull request is submitted, you'll notice that you can't immediately merge it (i.e., have your changes added to the source code). This is because we enforce a mandatory code review from a maintainer prior to merging. Code review is an essential part of development. What it means is that someone who owns or manages the repository looks over your changes to make sure that nothing breaks, everything works as expected, and that - in some cases - your coding style is up to the project's standards. | ||
|
||
### Code Review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feel free to link to the official GH docs for this as well.
### Create a New Branch | ||
|
||
We want to make sure that changes we make don't impact other people, or at least not until those changes are done. Out of courtesy, we'll develop our changes on our own "branch", which is a version of our code. | ||
|
||
The default branch is called `master`, and it contains the code that goes live on [teachla.uclaacm.com](https://teachla.uclaacm.com). We don't want to touch that just yet, so we're going to make our own branch! | ||
Default branches are either called `master` or `main`, and they contain the code that goes live on our websites' links! For instance, `teach-la-website`'s master branch code can be seen on [teachla.uclaacm.com](https://teachla.uclaacm.com). We don't want to touch that just yet, so we're going to make our own branch! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the future, we should shift all of our branches to main
- makes writing this a lot easier.
``` | ||
|
||
If you already have node installed in your machine, you should see some | ||
numbers in the form of `vXX.XX.X` which means that you already have node (and consequently npm) installed, and don't need to do anything else! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might want to add something about npm -v
too!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, we should probably mention something about using the right node version - esp with node 16, it really does matter which version they install.
To check that node has been properly installed on your machine, just run: | ||
|
||
```sh | ||
$ node -v | ||
``` | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this duped?
|
||
```sh | ||
$ git checkout -b my-branch-name # create a new branch and check it out | ||
$ jekyll serve # set up our dev server | ||
$ git clone https://github.com/uclaacm/teach-la-website.git |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ooh, I'd use the ssh clone by default if possible. Might be good to recommend the ssh keygen stuff as well in the dev setup.
|
||
```sh | ||
$ git checkout -b my-branch-name # create a new branch and check it out | ||
$ jekyll serve # set up our dev server |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be bundle exec jekyll serve
@matthewcn56 any way i can help? |
So sorry about that, will finish this up this weekend! Just have to move around a couple files |
Oh no worries! didnt know if you were waiting on vivian and i could help or something :)) |
Our projects like the Editor and the Learning Labs will need either npm or yarn for package management, so this adds in the info needed to install them inside of
dev-setup.md