Managing listPath's impossible null state
#37
Conversation
|
Visit the preview URL for this PR (updated for commit 74b3e38): https://tcl-77-smart-shopping-list--pr37-tg-list-path-41vmk85i.web.app (expires Mon, 30 Sep 2024 23:59:03 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: b77df24030dca7d8b6561cb24957d2273c5e9d72 |
tannaurus
changed the title
listPath's possible null statelistPath's impossible null state
| ); | ||
| }; | ||
|
|
||
| if (!listPath) { |
There was a problem hiding this comment.
I see that the Header handles the retruning early if listPath is null or falsy so that the components that require listPath to be defined don't have to do the checks themselves...
I think I am trying to wrap my head around why do that check here in the List & ManageList instead of higher in the App, would it have over complicated the logic or does it make conceptual sense to put it here for both of them?
There was a problem hiding this comment.
We probably could have made those routes re-direct if listPath was null, but that would be changing existing behavior in the app. We've been displaying these routes + header if there was no listPath, and I didn't want to take the liberty of changing that because I think that should be the team's decision. It could be thought of as part of the project you had mentioned some interest in; there's a good amount of overlap between "what does this look like when you're not-signed in" and "what does it look like when you first sign in"
Might be something worth raising to the team!
There was a problem hiding this comment.
That makes total sense, I didn't think about how we have had specific functionality and how that could change the app. Thinking about how the overall process/usage flow of the app is a consideration in where the listPath can be null and can't be.
There was a problem hiding this comment.
I don't think I ever caught the bug in my testing with the list so it is awesome for it to be caught here!
I didn't really know where to put this just a random question: would us using the local storage of the listPath after it is set would this change how all of the passing of the listPath prop is handled?
| @@ -8,11 +8,21 @@ interface Props { | |||
| } | |||
|
|
|||
| export function ManageList({ listPath, data }: Props) { | |||
| return ( | |||
| <div> | |||
| const Header = () => { | |||
There was a problem hiding this comment.
I like this like internal component usage in creating the header. I've never really done that usually just the full pull out of a component but this is a cool thing to keep top of mind.
There was a problem hiding this comment.
I personally do things like this when it's a super simple thing, like a few lines that happen to repeated a few times in a given component. If it becomes any more complex, and especially if there are hooks invalid, it's much butter to move it out of the parent component. When it's this simple though, retaining the context of the parent is really nice (I don't have to name it "ManageListHeader" because it's defined inside "ManageList")
No sweat! That's actually how I discovered this bug in the first place, I was in the process of unraveling why |
Description
This fixes two things:
listPathbeingstring | nullin places where it could have been prevented already (re: making impossible states impossible). These changes take the approach of ensuringlistPathis notnullat the parent route level so that all sub-components don't have to contend with handling an impossiblenullstate.Listpage would render a component that suggested your list was just empty and redirect you to "add items to the list" which, upon being redirected, there was nothing to do (because there was no list to begin with)