-
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
Render a user prompt and add item button when a shopping list is empty #22
Conversation
Visit the preview URL for this PR (updated for commit 6548725): https://tcl-57-smart-shopping-list--pr22-es-bm-empty-list-pro-devszmzz.web.app (expires Mon, 08 May 2023 04:28:39 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: ad3eb6c34c2ec5986fcc218178df5985eb9c9ffb |
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.
Awesome improvements to the UI and UX! These change make for a much more inviting experience. Great work!
Two minor improvements for the future related to UI/UX but that are probably outside the scope of this issue:
- prevent rendering the
List
page until the application has finished querying for a token's list items. It looks theList
page is rendering prematurely. As a result, the user is met with "Your list currently has no items" for a brief second even if their list does in fact have items (see attached video).
Screen.Recording.2023-04-27.at.2.26.17.PM.mov
- redirect the user to the
Add Item
page if they have an empty list (if they either just created a new list or they joined an empty list) instead of theList
page. Currently, if they have an empty list they're redirected to theList
page and once there, they're instructed to actually go to theAdd Item
page instead.
src/views/Home.jsx
Outdated
@@ -34,14 +34,17 @@ export function Home({ setListToken }) { | |||
|
|||
return ( | |||
<div className="Home"> | |||
<h2>Welcome to your Smart Shopping List! </h2>{' '} |
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've never seen the {' '}
at the end of a line here in JSX. What does this line look like without it? And could this be a spacing issue better controlled with CSS?
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 question, Amy. Beth and I were wondering the same, as these kept appearing this week. I'm going to do a little research and see what I can find out about this.
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.
It adds whitespace, and I don't know, but it's possible the linter adds this when you save. I notice when I use Prettier this can happen.
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.
Agree with what @danathedev. Sometimes if I copy/paste or move things around in the code, that whitespace gets add unintentionally. Probably safe to remove!
src/views/List.jsx
Outdated
<h3> Here are the items in your list:</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.
This is really elegant, nice job. I had no idea you could use an inline conditional that way.
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.
Nice Job, Beth and Emily! A few notes:
On my screen the clear button on the List view gets moved to the next line. Maybe that can be controlled later with CSS. Or another option to shorten the name of the button .. maybe "clear search" or "clear search box"
Also, and this is probably out of scope, but I wonder if later on down the line, we want the user to have the option to return back to Home to create another list if they want to. Currently cannot navigate back after creating the list.
Well done.
Thanks, @jongranados! Agreed on both counts, those fixes would definitely improve the UX - nice catch! |
Thanks, @Amy-Pr! Great points there. I wonder if more concise button text might be good practice in general. And I agree that adding the option to return home to create another list would improve the app. I'll add TODO comments now for both of these items. |
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.
Good work this week! Thanks for your pr and collaborating! The app is coming along. You should be proud. 😁
src/views/List.jsx
Outdated
{renderedListLength === 0 && ( | ||
<> | ||
<h2>Your list currently has no items. </h2> | ||
<h3>Click on the add first item button to start your list.</h3> | ||
<Link to="/add-item"> | ||
<button> add first item</button> | ||
</Link> | ||
</> | ||
)} | ||
{renderedListLength > 0 && ( | ||
<> |
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.
Good job. You see this all the time in React, with the short-circuiting. You have choices here. You could also choose to render with an if/else (who would, but hey, it's an option), OR a ternary. {renderedListLength > 0? (return some JSX) : (return this other JSX)}
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 started writing this exact thing before I saw that Dana had already said it! Great minds.
IMO, all of these choices for how to do conditional rendering come down to readability. Do the thing that makes the code easiest to understand!
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.
Thanks, @danathedev and @mxmason, for the guidance here! This has been refactored with a ternary.
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.
Awesome job, @emilysellers and @bethmelmtv! Just a minor comment about removing the extra whitespace added. Great work this week!
src/views/Home.jsx
Outdated
@@ -34,14 +34,17 @@ export function Home({ setListToken }) { | |||
|
|||
return ( | |||
<div className="Home"> | |||
<h2>Welcome to your Smart Shopping List! </h2>{' '} |
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.
Agree with what @danathedev. Sometimes if I copy/paste or move things around in the code, that whitespace gets add unintentionally. Probably safe to remove!
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 left a bunch of little UX copywriting notes for you to consider. Great work!
src/views/List.jsx
Outdated
{renderedListLength === 0 && ( | ||
<> | ||
<h2>Your list currently has no items. </h2> | ||
<h3>Click on the add first item button to start your list.</h3> | ||
<Link to="/add-item"> | ||
<button> add first item</button> | ||
</Link> | ||
</> | ||
)} | ||
{renderedListLength > 0 && ( | ||
<> |
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 started writing this exact thing before I saw that Dana had already said it! Great minds.
IMO, all of these choices for how to do conditional rendering come down to readability. Do the thing that makes the code easiest to understand!
<label htmlFor="search-filter"> | ||
Search for an item in your list: | ||
</label> |
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.
It's usually a good idea to make our labels as short as possible. This saves space in our UI, eliminating a problem that Amy pointed out in her screenshot, and reduces clutter for users who are listening to our page via screen reader. Why not "Search" here?
Generally, you will find that labels for text inputs, radios, and the like can be longer if they have to, but button labels are very short. I once worked on a team where buttons could only contain two words or fewer, for example.
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.
thank you EJ for this! This is good to know! I will update the button to "search"
src/views/Home.jsx
Outdated
<label htmlFor="tokenInput"> | ||
enter three word token to view existing list: | ||
</label> | ||
<label htmlFor="tokenInput">enter three word token:</label> |
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.
When we're labeling controls (elements that the user interacts with), we're generally not writing commands to the user; we're writing descriptions of the input we want. In this specific case, we can delete the word "Enter" from this text, turning it from a command to a description.
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.
Oh that's so good to know EJ! Thanks for this tip! I'll update this as well :)
src/views/Home.jsx
Outdated
@@ -50,10 +53,9 @@ export function Home({ setListToken }) { | |||
required | |||
/> | |||
|
|||
<button type="submit">Submit</button> | |||
<button type="submit">submit</button> |
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 recommend changing submit
to Submit
. Generally, labels are written in sentence case (I am a sentence
), and not lower case or Title Case
.
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.
thanks for this Ej! It's updated now
…ab-lab/tcl-57-smart-shopping-list into es-bm-empty-list-prompt
thank you for this! we will remove the extra whitespace |
Description
This PR improves the UI by providing the user with guidance on how to get started with the app:
add first item
button which redirects to the add-item page.Related Issue
Closes #7
Acceptance Criteria
Type of Changes
Updates
Before
revised.mov
Before our changes, the app did not have explicit prompts to help new users navigate through the app to add their first items to their grocery list.
After
part2.mov
With the addition of text on the
Home
page and theList
page, new users now have a more guided experience in order to help them confidently add their very first item to their list.For users who have not yet started a grocery list, we've also added a button on the
List
view page to prompt the user to add a first item to their list.Testing Steps / QA Criteria