-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix selected list to display in dropdown #93
Conversation
Visit the preview URL for this PR (updated for commit 43faf72): https://tcl-67-smart-shopping-list--pr93-dropdown-display-sel-itg7eu1z.web.app (expires Thu, 11 Apr 2024 02:53:16 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 1dc6f6876568bd8a1c97781eec7984835c207f7c |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src/context/UserProvider.jsx
Outdated
@@ -6,6 +6,8 @@ import { UserContext } from './user'; | |||
export function UserProvider({ children }) { | |||
const [user, setUser] = useState(null); | |||
|
|||
console.log(user); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove
src/components/SelectListForm.jsx
Outdated
@@ -36,11 +40,6 @@ export default function SelectListForm({ | |||
<button onClick={() => navigate('/list')}>View List</button> | |||
</div> | |||
)} | |||
<div> | |||
<h3> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we want to keep this h3 inside the div and where it is originally. this is according to the lofi mockup layout.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We aren't using listPath that was sent in as props from Home. What if we sent in listName instead and used listName as the initial value of the useState for setSelectedList?
If !selectedList, we can set it to 'Select a list' so it's not blank
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes that's a good idea!! @aberger3647
…d unused listPath from props
…m/the-collab-lab/tcl-67-smart-shopping-list into dropDown-display-selected-list-69
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey Tish, still not seeing the expected behavior. I would make sure whatever is needed to display the listName in the select dropdown is actually getting passed to the correct component.
Also, when there is no list selected, it is showing a blank dropdown when it should display something like "Select a list", as Alex mentioned in her comment. You can take a look at her comment to see a suggested way of implementing this.
I would also recommend running through the user flow locally to make sure the expected behavior exists before re-requesting reviews. Thanks!
src/components/SelectListForm.jsx
Outdated
areListsLoading, | ||
listName, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to make sure this prop is actually being passed in. Where is it being passed in from?
src/components/SelectListForm.jsx
Outdated
setListPath, | ||
}) { | ||
const [selectedList, setSelectedList] = useState(''); | ||
const [selectedList, setSelectedList] = useState(listName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can set a default state here for if no list is selected, something like "No list Selected"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jtkabenni I tried this and it worked while on the Home page, but when I viewed the list and returned to the home page, it went back to that default state. The only way I have been able to make this work the way we expect is to set the default to listName, and add a condition in the component body:
if (!listName) { setSelectedList('No list selected') }
Currently, if I select a list and then go to that list, then return to the home page, it still shows "Select a list". See my reply to Jenny for a suggestion on fixing that behavior |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great solution!
src/components/SelectListForm.jsx
Outdated
setListPath, | ||
}) { | ||
const [selectedList, setSelectedList] = useState(''); | ||
const [selectedList, setSelectedList] = useState(listName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jtkabenni I tried this and it worked while on the Home page, but when I viewed the list and returned to the home page, it went back to that default state. The only way I have been able to make this work the way we expect is to set the default to listName, and add a condition in the component body:
if (!listName) { setSelectedList('No list selected') }
Description
This pull request displays the selected list name in the dropdown once selected by a user.
Related Issue
#69
Acceptance Criteria
Selected list name is displayed once clicked in dropdown.
Type of Changes
Bug fix
Updates
Before
After
Testing Steps / QA Criteria