Skip to content

Comments

MDN React Tutorials #4: Hooks and state#30977

Merged
chrisdavidmills merged 22 commits intomdn:mainfrom
mxmason:ej/react-rewrite-4
Dec 21, 2023
Merged

MDN React Tutorials #4: Hooks and state#30977
chrisdavidmills merged 22 commits intomdn:mainfrom
mxmason:ej/react-rewrite-4

Conversation

@mxmason
Copy link
Contributor

@mxmason mxmason commented Dec 13, 2023

Description

Following #30515, as part of mdn/mdn#474, this PR makes some modifications to the fourth page in our "Getting started with React" tutorial.


Do not merge this until #30514 has been merged and you have updated this PR with mdn main and checked for breakages.

@github-actions github-actions bot added the Content:Learn:Client-side Content under “Client-side JavaScript frameworks” (Svelte, React, Angular, Vue) and related subtrees label Dec 13, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Dec 13, 2023

Preview URLs

(comment last updated: 2023-12-21 11:27:18)

Copy link
Contributor Author

@mxmason mxmason left a comment

Choose a reason for hiding this comment

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

To you, @chrisdavidmills!

Copy link
Contributor

@chrisdavidmills chrisdavidmills left a comment

Choose a reason for hiding this comment

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

Brilliant work, thank you @mxmason! I've got a few suggestions for you to look through, but nothing major.

I also made a couple of small changes along the way, just minor text tweaks.

Copy link
Contributor Author

@mxmason mxmason left a comment

Choose a reason for hiding this comment

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

Back to you, @chrisdavidmills! This one got away from me a bit, but I think these changes are worthwhile.

Also, FYI: I've opened a PR against Mozilla's reference implementation of the todo list app: mdn/todo-react#97

Copy link
Contributor

@chrisdavidmills chrisdavidmills left a comment

Choose a reason for hiding this comment

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

@mxmason OK, re-reviewed! Just another couple of small suggestions and sets of small text tweaks for you to review. Very nearly there now!

Copy link
Contributor Author

@mxmason mxmason left a comment

Choose a reason for hiding this comment

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

Alright, @chrisdavidmills, I've made one more change. I'm good with this if you are!

```

This `on*` naming convention appears in several React projects, so keep it in mind as you continue your learning. For the sake of clarity, we're going to stick with `addTask` and similar prop names for the rest of this tutorial. If you changed any prop names while reading this section, be sure to change them back before continuing!
This `on*` naming convention is very common in the React ecosystem, so keep it in mind as you continue your learning. For the sake of clarity, we're going to stick with `addTask` and similar prop names for the rest of this tutorial. If you changed any prop names while reading this section, be sure to change them back before continuing!
Copy link
Contributor Author

Choose a reason for hiding this comment

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

🔍 It is not an exaggeration to say that I have seen the on* callback pattern in every React project I've ever worked in, so "several" is not strong enough language. I've adjusted here and also deleted the mention of the broader ecosystem a couple paragraphs up (see 3acf664)

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, fair enough. I think your new language here is better. I think I was a bit worried about using language like "vast majority", as people might think "then why the heck did they not use it here?". But I'm happy with this update.

Copy link
Contributor

@chrisdavidmills chrisdavidmills left a comment

Choose a reason for hiding this comment

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

@mxmason ding ding! Approved ;-)

@chrisdavidmills chrisdavidmills marked this pull request as ready for review December 15, 2023 08:58
@chrisdavidmills chrisdavidmills requested a review from a team as a code owner December 15, 2023 08:58
@bsmth bsmth removed the request for review from a team December 20, 2023 10:26
@chrisdavidmills chrisdavidmills changed the title DO NOT MERGE: MDN React Tutorials #4: Hooks and state MDN React Tutorials #4: Hooks and state Dec 21, 2023
@chrisdavidmills chrisdavidmills merged commit 866c2ac into mdn:main Dec 21, 2023
dipikabh pushed a commit to dipikabh/content that referenced this pull request Jan 17, 2024
* chore: clean up intro

* chore: .js

* chore: fix import statements

* chore: small nit

* chore: disambiguate "e" as "event"

* chore: add "at this point" checks

* chore: fix final app.jsx check

* small tweak

* Small tweak

* Small tweak

* chore: suggestions from code review

Co-authored-by: Chris Mills <[email protected]>

* chore: clarify form submission section

* chore: add naming convention note

* chore: rewrite state section

* chore: nits

* Small tweak

* Small tweaks to new section

* Small tweaks to updated state section

* chore: apply suggestions from code review

Co-authored-by: Chris Mills <[email protected]>

* chore: clarify ecosystem statement

---------

Co-authored-by: Chris Mills <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Content:Learn:Client-side Content under “Client-side JavaScript frameworks” (Svelte, React, Angular, Vue) and related subtrees

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants