Issue #2: Render the List and list items a user has access to #20

zahrafalak · 2024-08-14T16:54:09Z

For an example of how to fill this template out, see this Pull Request.

Description

The most important feature of our app is the ability to look at their shopping lists to see what's on them, so the user knows what they need to buy.

Acceptance Criteria

Acceptance criteria

The useShoppingLists hook is used in App.jsx to get the shopping lists a user has access to from the database
The lists are passed into the Home component as a prop named data
The setListPath function is passed into the Home component so we can update the current list
The useShoppingListData hook is used in App.jsx to get the items in the current list from the Firestore database
The list is passed into the List component as a prop named data
In Home.jsx, the SingleList component is used to render the name of each shopping list a user is subscribed to
In List.jsx, the ListItem component is used to render the name of each item

Type of Changes

Rendered List name and items a user has access to. Feature.

Updates

After

github-actions · 2024-08-14T16:55:16Z

Visit the preview URL for this PR (updated for commit 136cf25):

https://tcl-77-smart-shopping-list--pr20-fz-rc-read-lists-ite-ce8hiwiv.web.app

_{(expires Sun, 25 Aug 2024 21:13:30 GMT)}

_{🔥 via Firebase Hosting GitHub Action 🌎}

_{Sign: b77df24030dca7d8b6561cb24957d2273c5e9d72}

src/views/Home.jsx

eternalmaha

Hey guys! This code looks very clean and readable! Nice job! 🫡. One I got curious about was the decision to use “data.length !== 0 “ . Negative lengths I believe are impossible haha, and this is really just semantics, but what do you guys think about the decision to check “data.length > 0 “ so it explicitly checks whether the length is greater than 0? I’m curious about your thoughts!

bbland1 · 2024-08-16T22:56:47Z

Hey guys! This code looks very clean and readable! Nice job! 🫡. One I got curious about was the decision to use “data.length !== 0 “ . Negative lengths I believe are impossible haha, and this is really just semantics, but what do you guys think about the decision to check “data.length > 0 “ so it explicitly checks whether the length is greater than 0? I’m curious about your thoughts!

I do like the way that data.length > 0 since they do the same check it is like a nicer read on my eyes but I do think both ways are valid and work. I can't think of any like performance benefits or issues on either.

zahrafalak · 2024-08-17T00:59:57Z

Hey guys! This code looks very clean and readable! Nice job! 🫡. One I got curious about was the decision to use “data.length !== 0 “ . Negative lengths I believe are impossible haha, and this is really just semantics, but what do you guys think about the decision to check “data.length > 0 “ so it explicitly checks whether the length is greater than 0? I’m curious about your thoughts!

Thanks for the kind words and great question! You're absolutely right—negative lengths are impossible, so checking data.length > 0 would work just as well and might even be a bit clearer. We went with data.length !== 0 to explicitly highlight that we're checking for non-empty data, but I'm totally open to using the > 0 approach for clarity. Appreciate the feedback!

adidalal · 2024-08-17T01:27:52Z

src/views/Home.jsx


 export function Home({ data, setListPath }) {
+	const dataPresent = data.length !== 0;


One option is to inline this check and inverting it

So use: (!Array.isArray(data) || data.length == 0)

if we're worried about the array being null, you can use the "nullish coalescing operator" to give the prop a default value

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Nullish_coalescing

let list = null; // assign a default at the top of our component let listProp = list ?? []; console.log(listProp) // logs "[]"

I've seen the nullish coalescing operator before but didn't have a full grasp on how. Thanks for the docs and an example it makes so much more sense!

tannaurus · 2024-08-17T14:32:40Z

src/views/Home.jsx

-				 * so we can see which lists the user has access to.
-				 */}
+				{dataPresent
+					? data.map((list, index) => (


Anytime you see a "else, null" in a ternary, you can do something like this instead

dataPresent && <MyComponent />

It's a nice way to express the same behavior with less code 😄

https://legacy.reactjs.org/docs/conditional-rendering.html#inline-if-with-logical--operator

I also love when variables are named with a touch more context, like something like hasLists instead of dataPresent. This is especially helpful in languages like JavaScript that don't have compile time type checking. Naming a variable in a way that communicates to the reader what this "data" is can go a long way in maintaining a healthy codebase 🚀

+1, nice refactor recommendation Tanner! Especially since we're not using TypeScript here I agree hasLists is a clear name for readability in the future.

Love the recommendations and suggestions. Thanks so much for the feedback!

alex-andria

Great work overall on your first repo! 🥳🌟🌟 I loved reading the discussions, questions, and recommendations on this PR. I see there are a few potential areas to refactor if you decide to update those before merging to main - which is totally normal in PR reviews and what these are for 🙌. Nice job making the code simple and clean as well.

My suggestion for writing your next PR is to include the Testing Steps / QA Criteria like how Briana and Maha written it for their PR in Issue #1. This makes it easier for those of us testing the code in our local repos to know what changes to look/test for, especially as the issues get more complex throughout the cohort. Nice work Falak and Ross!

src/views/Home.jsx

alex-andria · 2024-08-17T20:48:53Z

src/views/Home.jsx

-				 * so we can see which lists the user has access to.
-				 */}
+				{dataPresent
+					? data.map((list, index) => (


+1, nice refactor recommendation Tanner! Especially since we're not using TypeScript here I agree hasLists is a clear name for readability in the future.

alex-andria · 2024-08-17T20:54:12Z

src/views/List.jsx

@@ -1,17 +1,16 @@
 import { ListItem } from '../components';

 export function List({ data }) {
+	const dataPresent = data.length !== 0;


I do notice that dataPresent is a variable that is declared in both Home.jsx and List.jsx. Something to consider for cleaner code either now or in a later issue is to declare this variable at a higher file level, like maybe App.jsx. And pass that variable into Home and List so that you don't have to declare it each time.

That is a great callout! By my understanding, the 'data' prop passed onto these two components contain different information, even if the name of the prop is same. Do I understand that correctly? I guess that adds even more value to the suggestion above for using more contextual variable names like 'hasLists' or 'hasItems'.

RossaMania · 2024-08-18T21:13:55Z

I'm so glad we were able to change the var names to provide context! The in-line && operator makes things even easier to read! And all the checks passed! Yay!

zahrafalak added 2 commits August 14, 2024 10:33

Render name of each shopping list a user has access to

879fdef

Render name of each item in a list

26023d4

zahrafalak requested review from eternalmaha and bbland1 August 14, 2024 16:54

zahrafalak assigned RossaMania and zahrafalak Aug 14, 2024

bbland1 reviewed Aug 15, 2024

View reviewed changes

src/views/Home.jsx Outdated Show resolved Hide resolved

eternalmaha approved these changes Aug 16, 2024

View reviewed changes

bbland1 approved these changes Aug 16, 2024

View reviewed changes

zahrafalak requested review from adidalal, tannaurus and alex-andria August 17, 2024 01:01

adidalal reviewed Aug 17, 2024

View reviewed changes

adidalal approved these changes Aug 17, 2024

View reviewed changes

tannaurus reviewed Aug 17, 2024

View reviewed changes

alex-andria approved these changes Aug 17, 2024

View reviewed changes

Use more contextual variable names, and add inline logical && operator

136cf25

zahrafalak merged commit b83e38b into main Aug 18, 2024
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #2: Render the List and list items a user has access to #20

Issue #2: Render the List and list items a user has access to #20

zahrafalak commented Aug 14, 2024 •

edited by bbland1

Loading

github-actions bot commented Aug 14, 2024 •

edited

Loading

eternalmaha left a comment

bbland1 commented Aug 16, 2024

zahrafalak commented Aug 17, 2024

adidalal Aug 17, 2024

tannaurus Aug 17, 2024

bbland1 Aug 17, 2024

tannaurus Aug 17, 2024 •

edited

Loading

tannaurus Aug 17, 2024

alex-andria Aug 17, 2024

zahrafalak Aug 18, 2024

alex-andria left a comment

alex-andria Aug 17, 2024

alex-andria Aug 17, 2024

zahrafalak Aug 18, 2024

RossaMania commented Aug 18, 2024


		export function Home({ data, setListPath }) {
		const dataPresent = data.length !== 0;

Issue #2: Render the List and list items a user has access to #20

Issue #2: Render the List and list items a user has access to #20

Conversation

zahrafalak commented Aug 14, 2024 • edited by bbland1 Loading

Description

Acceptance Criteria

Type of Changes

Updates

After

github-actions bot commented Aug 14, 2024 • edited Loading

eternalmaha left a comment

Choose a reason for hiding this comment

bbland1 commented Aug 16, 2024

zahrafalak commented Aug 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tannaurus Aug 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alex-andria left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RossaMania commented Aug 18, 2024

zahrafalak commented Aug 14, 2024 •

edited by bbland1

Loading

github-actions bot commented Aug 14, 2024 •

edited

Loading

tannaurus Aug 17, 2024 •

edited

Loading