Issue #43 Users should be able to view the list name #44

eternalmaha · 2024-09-25T16:01:56Z

Description

Users should be directly taken to the list they are viewing and should be able to view the name of the list at the top view.

Before this PR, users needed to select a shopping list, and then click the "list" navigation bar, which is an unnecessary step. Once taken to the list, the list name did not appear anywhere on the page view, creating possible confusion.

This issue explored the routing structure in the application. Initially, I attempted to simply adjust the useNavigation call for each list in the SingleList by appending it from \list to "\list{path}. However, it created authentication issues because our protected routes were set to only allow access to the Lists from the \list navigation bar. I needed the path to be dynamic based on the selected list. I learned and chose to use dynamic routing, which allows the use of a parameter as a place holder in the URL path. With dynamic routing in place, I was able to finally append the navigation in SingleList with the List Name (which was already being passed to the SingeList component). I learned I could capture the value of a URL path stored in the dynamic route parameter through the react hook useParams(). useParams() will capture and display the users' current selected list's name because the list name was set up as the dynamic route parameter.

Related Issue

closes #43

Acceptance Criteria

The user is immediately redirected to the list after clicking the name of each list.
The name of the list appears at the top of each list view when a user clicks.
The user is still able to view each list from the mange-list view.

Type of Changes

enhancement

Before

After

navigate.to.list.item.mov

Testing Steps / QA Criteria

Sign into application
The home view will display the added shopping lists.
Click on any shopping list.
User should be directly navigated to the specified list.
List name will appear at the top of the page view.

github-actions · 2024-09-25T16:03:04Z

Visit the preview URL for this PR (updated for commit 4100e57):

https://tcl-77-smart-shopping-list--pr44-ma-view-list-name-vmt1pg1k.web.app

_{(expires Fri, 04 Oct 2024 02:45:40 GMT)}

_{🔥 via Firebase Hosting GitHub Action 🌎}

_{Sign: b77df24030dca7d8b6561cb24957d2273c5e9d72}

…pecified list

…name from the specifeid url path of each list

zahrafalak · 2024-09-27T05:08:54Z

src/App.tsx

@@ -65,7 +65,7 @@ export function App() {
 					{/* protected routes */}
 					<Route element={<ProtectRoute user={user} redirectPath="/" />}>
 						<Route
-							path="/list"
+							path="/list/:listName"


One thing I noticed as a side effect is that the list option in the navigation bar now shows a "Page Not Found" error when clicked. The same happens when I click on View List button on the ManageList view. It might be happening due to this part, because we do not have a route defined for just /list anymore, so the existing navigation functions in other components don't work as expected.

I'm curious about the idea of placing all the singlelist elements into the list view altogether. Then once each list is clicked, it'll redirect to its own view? I might need to rethink the routing there. I feel like at some point, we'd need to transfer the lists from the home view for better design so the user isn't bombarded with a list immediately upon signing. What do you guys think about placing the "share list' with friends/family into each individual list view? So the user is able to see the actual list before sharing it. This would probably be its own issue. I wonder the full scale of that update.

Whewww! So I was able to fix the issue of view list redirected to the 404 no page found since the \list view is out of commission for now. The name of the list is included in the end of the listPath parameter we are passing in the addItem form. I extracted and stored the name of each list from the listPath using the good ol - fashion pop() method! Using string interpolation, I appended our useNavigate in the addItem form to include the name. Now we should successfully still be able to navigate to the list when clicking the "view list" button on the addItem form!

Now we need to explore the issue of 404 no page found with \list view.

if we end up wanting to keep the /list route we should just be able to have botht he one with the params and the basic list route as long as we order them properly. I beleive if we have the non param route first it would still work for both routes as we intend.

suggestion <Route path="/list" lement={<List data={data} listPath={listPath} />} /> <Route path="/list/:listName" element={<List data={data} listPath={listPath} />} />

I'm noticing a 404 page when a new list is created:

Towards @alex-andria 's observation, the same behavior happens when you try to navigate to the list via the List nav bar link. To fix that bug, we need to:

Remove List from the nav bar (it no longer serves a purpose imo)

Update CreateList.handleSubmit to navigate to /list:listName

i fixed the issue of the create list function leading to a 404 no page found. I wasn't able to navigate to /list:listName because only because our List component route path is set to use the paramater :listName as the list name at the top of the page view and :listName appears at the top that way! I had to manipulate the "path" string to grab the name from it.

On the note of the "list" nav bar. It seems like the majority of the team is okay with removing the list nav bar button from the application since it no longer serves its purpose of navigating to the lists. I believe this is a separate issue, and am hesistant to remove it now unless everyone is okay with it at this point.

@eternalmaha I'm personally gonna vote/say go ahead and remove it. I feel like this PR really progressed the functionallity of the list name button and made the nav bar list a bit redundant and I think it would be ok to do here as well.

bbland1 · 2024-09-27T16:13:10Z

src/views/authenticated/List.tsx

@@ -1,3 +1,4 @@
+import { useParams } from "react-router-dom";


suggestion: On line 6 useNavigate is imported from react-router-dom again and we get an error in the terminal from vite about dupilicate imports. We should be able to combine the 2 imports to stop that error, and get rid of the one on line 6.

Suggested change

import { useParams } from "react-router-dom";

import { useParams, userNavigate } from "react-router-dom";

bbland1 · 2024-09-27T16:20:52Z

src/views/authenticated/List.tsx

-				<p>
-					Hello from the <code>/list</code> page!
-				</p>
+				<p>{listName}</p>


nitpick (non-blocking): we could change this to some type of header tag. it maybe would mean adjusting the others on the page since
is used in the header but it may make it a more distinct part of the page.

Yes I agree! I do not think the listName should be in a paragraph element, that's very disrespectful to it!!! I'll adjust it to a
tag probably. It'll be fun to explore the styling our list names will have when we finalize our apps main styling theme soon!

I didn't realize it would put that text in as a header 😂

RossaMania

I like what you have done here! Even though the list name is in a paragraph element, it should be able to be styled with Bootstrap!

I do like the idea that now that the /list navigation buttons may not be needed, that it may be good to just have the /list route and buttons lead to a list of the lists. That, and the adding that ShareListForm component to each list as well. That ShareListForm component is tucked in components/forms.

As of right now, the /list nav button would no longer be needed. I wonder if all of this is scope creep and perhaps 1 or 2 new issues should be made instead?

After all, the /list route could be a list of lists and the home page could be styled up with Bootstrap and probably some sort of Call To Action thing----including a call to action to click on that list button to see the lists.

I consider this pull request approved.

bbland1 · 2024-09-28T05:27:21Z

src/views/authenticated/List.tsx

@@ -11,6 +11,7 @@ interface Props {

 export function List({ data: unfilteredListItems, listPath }: Props) {
 	const navigate = useNavigate();
+	const { listName } = useParams<{ listName: string }>();


praise: I like seeing useParams here. It is something that I've used very little so it i nice to see it used here and getting the use that you wanted in it! 🎉

I was very excited to learn about its existance when I was exploring routing concepts that were more flexible! Thanks for the appreciation!

bbland1 · 2024-09-28T05:39:14Z

src/components/SingleList.tsx

 	setListPath: (path: string) => void;
 }

 export function SingleList({ name, path, setListPath }: Props) {
+	const navigate = useNavigate();
+
 	function handleClick() {
 		setListPath(path);


suggestion: noticed that when clicking through lists when clicking one, going back to the home page and then clicking another list there is a half second moment where the old list can be seen. hopefully with the video that makes more sense. I think we could get rid of this setListPath since we are navigating directly to lists with params now we don't really need to set the listPath for the list nav button anymore.

Screen.Recording.2024-09-27.at.12.22.50.PM.mov

Yes noticed this too. I can also look more into it later tonight since this is a common routing issue.

The problem here is rooted in our usage of firestore's onSnapshot api. We're subscribing to events on the list, which allows us to view changes as they happen in real time (if I was looking at an item that I shared with Alex, and she made a change, I should see it without refreshing my list). The downside of this is we can't use Javascript Promises. We don't have a way of awaiting the data of useShoppingListData to be refreshed to the new dataset.

Most importantly, I don't believe this issue was introduced in this change, it was just revealed by this change. This change makes it so much easier/quicker to jump around the app, that we're seeing how the data is just refreshing relatively slowly (we don't have a way of implementing a "loading" state).

Put differently, I don't think this needs to be fixed in this PR because it wasn't introduced here. Maybe someone could take a look at this issue in a follow up? I suspect the solution will be pretty involved.

You'll need to do something like this

loading_suggestion.patch

(you can download that, put it in your project folder, and do git apply loading_suggestion.patch to see it applied to your branch) I believe that should work but I haven't actually tested it 😄

I really appreciate your guys' keen and thorough eyes!! I do see the lag issue. There's a brief article on firebase about the the idea of listeners. https://firebase.google.com/docs/firestore/query-data/listen that was a bit enlightening to read.

I tried downloading the file and ran the git command, but It didn't work on my end. The errors on my terminal suggest there's a mismatch between the lines of code in the file and the lines on my codebase.I can try again when this turns into its own issue if need be.

I had some question about the purpose of listpath being set in singelist if we decide to take it out. Since the listPath is being supplied from our firebase, this wouldn't affect the lists' having a list path for the shareList function if we removed the setList path in singlelist right or anywhere else? I see that the path is simply being passed to singeList in the home view component as a property, singleList doesn't actually contain the data for listPath. This path was then needed by the list nav button to find the list to display? Ah I see now. So I believe removing listPath would be okay then!!! That’s such a great observation @bbland1 !!!! (You just read my train of thought) 😮‍💨

bbland1 · 2024-09-28T06:05:57Z

src/views/authenticated/List.tsx

-				<p>
-					Hello from the <code>/list</code> page!
-				</p>
+				<h1>{listName}</h1>


nitpick (non-blocking): we may want to change the list name to h2 since the header name Smart Shopping List is already h1 and I guess I just had this idea that you are noly suppose to use 1 per page but as I type this I am not sure if that is a real "rule" or something I've just thought this whole time 😅🤔😂

I dont think thats a nitpick, I think thats valid! You mentioning this actually had me thinking about the placement of our app title "smart shopping list". We can talk about this more when we all discuss design work, but having the app name in place of the "home view" or as part as the nav bar somehow could be a good idea, like as a logo. Then the name of the list could be the central figure in the page view and take on the
tag?!

bbland1 · 2024-09-28T06:07:01Z

src/components/forms/AddItemForm.tsx

@@ -85,7 +85,10 @@ export function AddItemForm({ listPath, data: unfilteredListItems }: Props) {
 		}
 	};
 	const navigateToListPage = () => {
-		navigate("/list");
+		if (listPath) {
+			const listName = listPath.split("/").pop();


praise: this is a really cool way to get the listName from the path and a cool way to use split and pop

shout out to my past life of freeCodeCamp tutorial rabbit holes!

alex-andria

Nice job on taking on this list name feature that is super helpful Maha! It works as intended now we just have to iron out a few things. When it goes to a list view, maybe some kind of loading spinner or logo can appear so that the user isn't seeing the half second flash of a prior list. We also need to look into the 404 error, but I can do some more digging on that tonight.

alex-andria · 2024-09-28T15:00:16Z

src/App.tsx

@@ -65,7 +65,7 @@ export function App() {
 					{/* protected routes */}
 					<Route element={<ProtectRoute user={user} redirectPath="/" />}>
 						<Route
-							path="/list"
+							path="/list/:listName"


I'm noticing a 404 page when a new list is created:

alex-andria · 2024-09-28T15:06:18Z

src/components/SingleList.tsx

 	setListPath: (path: string) => void;
 }

 export function SingleList({ name, path, setListPath }: Props) {
+	const navigate = useNavigate();
+
 	function handleClick() {
 		setListPath(path);


Yes noticed this too. I can also look more into it later tonight since this is a common routing issue.

alex-andria · 2024-09-28T15:06:39Z

src/views/authenticated/List.tsx

@@ -11,6 +11,7 @@ interface Props {

 export function List({ data: unfilteredListItems, listPath }: Props) {
 	const navigate = useNavigate();
+	const { listName } = useParams<{ listName: string }>();


eternalmaha · 2024-09-29T04:28:13Z

Thanks everyone for all your thoughts and insight!!! I was thinking about new issue of lag between the old list disappearing and the new list appearing. Thinking about the need to "wait" for the new dataset to refresh, I immediately thought about our good old friend the setTImeout() function. I used a small amount of time for the delay to navigate to the list so there isnt a real noticable change to view the list. This might not be the permanent solution but it seems to be a short while fix that's doing what it needs to do.

setTimeout(() => {
navigate(/list/${name});
}, 200)

eternalmaha · 2024-09-29T04:50:09Z

@RossaMania Thanks Ross for the feedback! Im glad you agree that its a better user experience for the share list component to be viewed in the same arena as the list and its contents. That would be an issue I'd be interested in exploring soon. I can open that new issue soon including the one with the list nav bar removal.

bbland1 · 2024-09-29T08:14:37Z

src/components/SingleList.tsx

 	function handleClick() {
 		setListPath(path);
+		setTimeout(() => {


praise 🎉: i know it seems like there are some other ideas being discussed about how to hadnle the little life switch over delay, so if that changes 👍🏽 BUT i really want to acknowledge how smart and cool this use of the setTimeout is! Was just coming back to see any new chagnes and it is so smooth now!

bbland1 · 2024-09-29T08:20:34Z

src/components/CreateList.tsx

@@ -29,7 +29,10 @@ export function CreateList({ user, setListPath }: Props) {
 			toast.success("Success: Your New List is Created!");
 			// Delay for toast notification before redirecting
 			setTimeout(() => {
-				navigate("/list");
+				console.log(path);


whose that pokemon?!

it's a...

console.log!!

😂😅

LOL! I guess I didn't catch em all !!!

…pose

… singleList component

passing props to List to view list name error

36f0b9f

eternalmaha self-assigned this Sep 25, 2024

eternalmaha added 4 commits September 25, 2024 12:15

reverse changes

4ed1f8e

set up dynamic routing for the list names at the parent level

b378637

refactored SingleList component to navigate to directly navigate to s…

caed9f5

…pecified list

refactored List component to utilize useParams to retreieve the list …

4100e57

…name from the specifeid url path of each list

eternalmaha changed the title ~~Issue #15 Users should be able to view the list name~~ Issue #43 Users should be able to view the list name Sep 27, 2024

eternalmaha requested review from tannaurus, zahrafalak, RossaMania, bbland1, alex-andria and adidalal September 27, 2024 03:02

eternalmaha marked this pull request as ready for review September 27, 2024 04:47

zahrafalak reviewed Sep 27, 2024

View reviewed changes

bbland1 reviewed Sep 27, 2024

View reviewed changes

eternalmaha added 3 commits September 27, 2024 14:05

fix: duplicate react-router-imports

ee97375

fix: view list now navigates to each list in the manage-list view

c477916

style: list name is more distinctive part of page

80f57f9

eternalmaha added the enhancement New feature or request label Sep 27, 2024

RossaMania approved these changes Sep 27, 2024

View reviewed changes

bbland1 reviewed Sep 28, 2024

View reviewed changes

bbland1 approved these changes Sep 28, 2024

View reviewed changes

alex-andria reviewed Sep 28, 2024

View reviewed changes

fix: createList now routes to the created list view page

b8cc1f6

fix: user is no longer seeing the old list when clicking on a new list

eb24f83

bbland1 reviewed Sep 29, 2024

View reviewed changes

eternalmaha and others added 4 commits September 29, 2024 10:57

refactor: removing list nav bar button as it no longer serves its pur…

3a3845e

…pose

refactor: removing unnessary setListpath and path prop passing in the…

a8a8529

… singleList component

fix: fixing merge conflicts

e44b418

Merge branch 'main' into ma/view-list-name

f70830c

eternalmaha merged commit 36baf23 into main Oct 3, 2024
1 check failed

eternalmaha deleted the ma/view-list-name branch October 3, 2024 13:12

eternalmaha restored the ma/view-list-name branch October 3, 2024 13:47

eternalmaha mentioned this pull request Oct 3, 2024

Revert "Issue #43 Users should be able to view the list name" #57

Merged

		@@ -1,3 +1,4 @@
		import { useParams } from "react-router-dom";

	import { useParams } from "react-router-dom";
	import { useParams, userNavigate } from "react-router-dom";

Issue #43 Users should be able to view the list name #44

Issue #43 Users should be able to view the list name #44

Conversation

eternalmaha commented Sep 25, 2024 • edited Loading

Description

Related Issue

Acceptance Criteria

Type of Changes

Before

After

Testing Steps / QA Criteria

github-actions bot commented Sep 25, 2024 • edited Loading

zahrafalak Sep 27, 2024 • edited Loading

Choose a reason for hiding this comment

eternalmaha Sep 27, 2024 • edited Loading

Choose a reason for hiding this comment

eternalmaha Sep 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tannaurus Sep 28, 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

bbland1 Sep 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

is used in the header but it may make it a more distinct part of the page.

eternalmaha Sep 27, 2024 • edited Loading

Choose a reason for hiding this comment

tag probably. It'll be fun to explore the styling our list names will have when we finalize our apps main styling theme soon!

Choose a reason for hiding this comment

RossaMania 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

bbland1 Sep 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tannaurus Sep 28, 2024 • edited Loading

Choose a reason for hiding this comment

tannaurus Sep 28, 2024 • edited Loading

Choose a reason for hiding this comment

eternalmaha Sep 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eternalmaha Sep 29, 2024 • edited Loading

Choose a reason for hiding this comment

tag?!

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

eternalmaha commented Sep 29, 2024

eternalmaha commented Sep 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eternalmaha commented Sep 25, 2024 •

edited

Loading

github-actions bot commented Sep 25, 2024 •

edited

Loading

zahrafalak Sep 27, 2024 •

edited

Loading

eternalmaha Sep 27, 2024 •

edited

Loading

eternalmaha Sep 27, 2024 •

edited

Loading

tannaurus Sep 28, 2024 •

edited

Loading

bbland1 Sep 27, 2024 •

edited

Loading

eternalmaha Sep 27, 2024 •

edited

Loading

bbland1 Sep 28, 2024 •

edited

Loading

tannaurus Sep 28, 2024 •

edited

Loading

tannaurus Sep 28, 2024 •

edited

Loading

eternalmaha Sep 29, 2024 •

edited

Loading

eternalmaha Sep 29, 2024 •

edited

Loading