Skip to content

Conversation

BartoszKlonowski
Copy link
Collaborator

@BartoszKlonowski BartoszKlonowski commented Oct 16, 2024

This pull request modifies the recap section in "Conditional Rendering" page.


Reasoning for this PR:
In the page the recap of conditional rendering with AND operator says:

In JSX, {cond && } means “if cond, render , otherwise nothing”.

while it is not always true. As we know, it will render nothing only if cond is one of null, undefined or "" (empty string).

I'm aware that previously the pitfall of putting numbers in AND conditional render is described very well, but I believe the recap should not be forgetting about it.
I fear that simplified thought in the recap is what will stay in one's mind leaving a conclusion that it's always fine to, almost with the muscle memory, use the AND to conditionally render.


I also allowed myself to edit the formatting, as I found the updated sentence being quite long, so having it in new line (I believe) improves the readability. Let me know what is your opinion.

Copy link

vercel bot commented Oct 16, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
19-react-dev ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 16, 2024 9:12am
react-dev ✅ Ready (Inspect) Visit Preview Oct 16, 2024 9:12am

Copy link

github-actions bot commented Oct 16, 2024

Size changes

📦 Next.js Bundle Analysis for react-dev

This analysis was generated by the Next.js Bundle Analysis action. 🤖

This PR introduced no changes to the JavaScript bundle! 🙌

Copy link
Member

@sophiebits sophiebits left a comment

Choose a reason for hiding this comment

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

I'm sorry but I don't think this is clearer overall.

To the new user who is just learning about conditionals for the first time, they would likely not think to put anything that is not a boolean on the left side, and the note is only extra noise for them.

For someone who read the page above, the Pitfall for numbers is called out clearly, but it's not an essential part of the page content for someone who is just learning JSX and conditional rendering.

The recap is intentionally very brief so that it is easy to grok. If you want to do anything to nudge people towards the ? : null construction, I am open to adding a sentence like Alternatively, {cond ? <A /> : null} means the same thing. but I am not sure it is necessary, and if we add it we would also need to revise the Conditional (ternary) operator (? :) section earlier to mention the same ability.

Especially in the intro docs, our aim is simplicity and understandability, and it is ok if we skip over some edge case details. Shorter pages are easier to understand. I might feel differently if this was one of the later pages (eg: a guide on Effects) but this is part of someone's very first steps with React and JSX and we don't want to overwhelm.

Copy link
Member

@rickhanlonii rickhanlonii left a comment

Choose a reason for hiding this comment

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

Instead, what if we added a bullet point that says:

  • In JSX, {0 && <A />} renders "0", avoid numbers on the left side of &&.

@BartoszKlonowski
Copy link
Collaborator Author

Instead, what if we added a bullet point that says:

  • In JSX, {0 && <A />} renders "0", avoid numbers on the left side of &&.

I would vote for this ☝️
My goal when making this PR was to make sure that in the recap we don't leave any inaccuracy, so this still would do.

@sophiebits
Copy link
Member

I still don't think that is essential knowledge and this is a very intro post but I won't block it

another alt:

if cond is true, render <A />; if false, render nothing

this is perfectly correct (does not specify what happens in the non-true/false case) but comprehensible to someone who is not caring about it

@rickhanlonii
Copy link
Member

Yeah I agree with @sophiebits I think the pitfall section is enough and this isn't necessary

@BartoszKlonowski
Copy link
Collaborator Author

Alright, so closing.
Thanks for the discussion 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants