Briana's feedback: round 5

[brianamarie]

Hi @partyshah - I'm going to make a list here. Some of the items are duplicates of previous feedback so I'm trying a different format.

• Consistent punctuation in step description in config
• Description of Readme
  • git > Git
  • Javascript > JavaScript
• Activity: open a pull request - needs formatted, included in 1st step
  • This should also be a short link that they just need to click
• Step 2: "App.jsx" > "src/App.jsx"
• Step 2: Do we need to save the file? Yes. Also in every step where they edit the file.
• Template repository doesn't have descriptive commits - and it needs to
• Step 2: "Added" > "Add", commits should be present tense
• How to Talk to your Child
  • Header is confusing, what does it mean? Too much jargon too soon
  • Inconsistent capitalization
  • Remove "Hmmmm" overly casual
  • Definition of props should be somewhere here, clearly formatted so it sticks out. Same thing for the definition of components earlier on.
• Step 3
  • "List.jsx" > "src/List.jsx". Remove this sentence it makes it seem like the file we edit is List instead of App
  • At the top? Which line? It's 3. Every single time we tell them to change a file it should say the line number. Apply this comment to every single response. Every single one.
  • Import statement reference in the template repository is step 2 instead of 3
• package-lock.json is sohwing as changed, we need to address that.
• Step 4: "Now," > "Now"
• If we're expected to keep the program running we should A) tell them how to exit, B) tell them explicitly when and why to open a 2nd window
• Step 4, Images #1: Which line?
• Step 4: "App.jsx" > "src/App.jsx". Apply this logic to every single response also.
• Step 4: Consistent punctuation at the end of steps
• First push should be git push -u origin changes, and all subsequent pushes should just be git push
• Step 4: Code explanation needs to go above activity and have a more specific title
• Step 5: Include directory paths in file titles
• Step 5, Images #1: Find a more concise way to say this step
• When replacing code, "{}" are confusing throughout in the template repository. Remove them.
• Step 5 First step triggered automatically #3, wrong file listed to commit.
• Many pushes and commits seem to use the same commit message instead of actually being descriptive
• All: I'll respond after you push > after I detect a push > formatting also needs fixed to match template
• "Content" as a header in one step still?
• Content section: Typo in dynamic
• Fix all occurrences: "Step 6 - sdfds" > "Step 6: sdfd" with a colon instead of a minus
• Step 6: Which line?
• Step 6: We have them check for errors but don't tell them what to do if there are errors - what's the point of this step? Remove it if we don't give them more information on what to do.
• Step 6: commit message isn't descriptive
• "Explaining this.setState": First person "I" edit
• 7: Which line?

At this point I stopped going through it because it felt like a lot of the feedback was repetitive. look forward to chatting about this @partyshah and seeing it implemented.

[brianamarie]

Related:
#31
https://github.com/github/learning-engineering/issues/585#issuecomment-512749356
https://github.com/github/learning-engineering/issues/585#issuecomment-514984668

[crichID]

@partyshah thank you for making the edits. I just went through the course and noticed a few functional things we may want to address:

1. In steps where I'm instructed to remove comments from lines, the bot proceeds even if I do not remove the comments
2. I'm able to incorrectly remove comments for other steps (for example, if remove the comments for step 9 when I am doing step 4) This leaves me in a state where I can't continue with the course because there is nothing for me to do in that step. This wouldn't be a problem if it wasn't so tempting because I can see all of the comments/code there in the file.

[hectorsector]

@partyshah I also reviewed the course (based on 5d7b594) and here are some notes I took that might help get it back on track:

• the template repo needs a little rewriting; it's bringing in commits about preparing the course for open release which the learner doesn't need to see in a PR
• in step 7: create a method is referencing diff that doesn’t exist
• in step 9: the line numbers referenced don't match what's actually expected from the learner

[hectorsector]

• the template repo needs a little rewriting; it's bringing in commits about preparing the course for open release which the learner doesn't need to see in a PR
• in step 7: create a method is referencing diff that doesn’t exist
• in step 9: the line numbers referenced don't match what's actually expected from the learner

These are done.

• In steps where I'm instructed to remove comments from lines, the bot proceeds even if I do not remove the comments
• I'm able to incorrectly remove comments for other steps (for example, if remove the comments for step 9 when I am doing step 4) This leaves me in a state where I can't continue with the course because there is nothing for me to do in that step. This wouldn't be a problem if it wasn't so tempting because I can see all of the comments/code there in the file.

@crichID these are done, if you have a chance to retest before we publish please do, but I've tested it 👍

@brianamarie thanks for the detailed feedback. I believe @partyshah went through a lot of it and addressed it. If you have an opportunity to re-review please do so and I'll take care of whatever you find.

[brianamarie]

Sounds good, @hectorsector. I'll try to test it again either today or tomorrow.

[hectorsector]

I think this was addressed by #53