Skip to content
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

7. Alert the user to avoid duplicate items on the list #53

Merged
merged 30 commits into from Jan 19, 2020

Conversation

@prophen
Copy link
Contributor

prophen commented Jan 7, 2020

Closes issue #29

Nikema Prophet and others added 7 commits Jan 7, 2020
Nikema Prophet
Nikema Prophet
…ow to work with database and check for duplicates. added some code with some help from friends
src/pages/AddItem.js Outdated Show resolved Hide resolved
src/pages/AddItem.js Outdated Show resolved Hide resolved
src/pages/AddItem.js Outdated Show resolved Hide resolved
Nikema Prophet and others added 7 commits Jan 12, 2020
Nikema Prophet
It mostly works but we need a way to make sure there is the duplicate is checked before sending the item to the database. I'm not super strong here but I think a callback function or promise can do it. I saw some things about useEffect also but I couldn't figure it out. I have been looking at this for too long.
@prophen prophen marked this pull request as ready for review Jan 12, 2020
@prophen prophen requested review from ajiles91 and MonicaDJohnson Jan 12, 2020
@prophen

This comment has been minimized.

Copy link
Contributor Author

prophen commented Jan 12, 2020

Our PR for story #53 is ready to review @ajiles91 @MonicaDJohnson

Copy link
Contributor

ajiles91 left a comment

Ok, I pulled the branch and ran it locally and everything behaved as expected. Nice work!

@prophen

This comment has been minimized.

Copy link
Contributor Author

prophen commented Jan 12, 2020

We found a bug so we're not actually ready for review @MonicaDJohnson @ajiles91. As it is currently, the code saves the item name as the document id. That makes it so that globally across everyone's lists we aren't able to have duplicate items.

ex: You put an apple on your list and now I can't.

Nikema Prophet added 5 commits Jan 16, 2020
In the checkForDuplicates function we take the item name and check to see if it's already on the shopping list. If it's a match it sets the duplicat state to true, the returns true. Instead of using state to determine if the item can be added we use the function return value. So duplicate items don't get added while waiting for the state to populate.
Nikema Prophet
…st() at the end of addItem() so that if a user rapidly enters duplicates the current list gets checked.
Nikema Prophet
Nikema Prophet
@prophen

This comment has been minimized.

Copy link
Contributor Author

prophen commented Jan 16, 2020

@ajiles91 @MonicaDJohnson we're ready for review 🤞🏾

Nikema Prophet added 2 commits Jan 16, 2020
src/pages/List.js Show resolved Hide resolved
Nikema Prophet
Copy link
Contributor

segdeha left a comment

NICE WORK!!! So great! I only have one small change I've requested to improve the UX. Otherwise, this is good to merge!!! 🙌

src/lib/normalizeName.js Outdated Show resolved Hide resolved
src/pages/AddItem.js Outdated Show resolved Hide resolved
Copy link
Collaborator

stevelikesmusic left a comment

Lots of great work! Using Context to share state across routes is a great idea.

Nothing additionally from me that would be a blocker. Mostly ideas and considerations regarding code structure and component APIs. @segdeha makes a great point for the UX about keeping the user input when an error is shown. Definitely something that can be ironed out before your call 👍

@@ -88,6 +108,12 @@ const AddItem = ({ firestore }) => {
</label>
</div>
</form>

{error ? (

This comment has been minimized.

Copy link
@stevelikesmusic

stevelikesmusic Jan 19, 2020

Collaborator

This is good for hiding/showing ItemError.

A more 'react' way of doing this would be to conditionally render the component:

{error && <ItemError />}

Which you can think of as:

if (error) {
  return <ItemError />
} else {
  return null;
}

This way, you don't need to worry about CSS hiding/showing an element. react knows when to render the component at all or not.

This comment has been minimized.

Copy link
@prophen

prophen Jan 19, 2020

Author Contributor

Thanks Steve. I knew there was an easier, React way to do this 😃. Addressed in this commit ba77d58


function ItemError(props) {
return (
<div className={'flash ' + props.className}>

This comment has been minimized.

Copy link
@stevelikesmusic

stevelikesmusic Jan 19, 2020

Collaborator

I like using an element like <aside> for error messages. Whenever possible, I try to find a semantic HTML element vs div.

Even better is to check if there's an ARIA role attribute that might fit for your component. In this case, I might reach for alert.

Considerations like this will make your apps accessible to more users—especially those using screen readers. While you can go overboard—screen readers announcing way too much to users—I think this case warrants giving a little bit more.

It's out of scope right now to dive into accessibility best practices. But wanted to at least call this out as something to keep in mind as continue building your skills.

One other thought might be to say what the item is (ex. "Oh no! Falafel is already on your list.")

if (shoppingList.length === 0) {
fetchList(token);
}
if (error) {

This comment has been minimized.

Copy link
@stevelikesmusic

stevelikesmusic Jan 19, 2020

Collaborator

Why the timeout?

This comment has been minimized.

Copy link
@prophen

prophen Jan 19, 2020

Author Contributor

Desperation 😃. When we were finally able to prevent duplicates from being entered, the error message just stayed there. Maybe if we implement the suggested changes that won't be an issue.

setError(false);
}, 2000);
}
}, [duplicate, error, fetchList, setError, shoppingList, token]);

This comment has been minimized.

Copy link
@stevelikesmusic

stevelikesmusic Jan 19, 2020

Collaborator

We can probably drop duplicate from the dependencies array.

const addItem = name => {
firestore.collection('items').add({ name, token, nextExpectedPurchase });
const addItem = () => {
if (!checkForDuplicates(name)) {

This comment has been minimized.

Copy link
@stevelikesmusic

stevelikesmusic Jan 19, 2020

Collaborator

It's common to call functions like this hasDuplicate(name).

The reason I prefer naming in this way is that I don't have to think about what the function is returning. isSomething, or hasSomething implies that it's a binary value (true or false).

setShoppingList(tempArray);
})
.catch(function(error) {
console.log('Error getting documents: ', error);

This comment has been minimized.

Copy link
@stevelikesmusic

stevelikesmusic Jan 19, 2020

Collaborator

I like that you're dropping an error message if we fail getting the list! It's out of scope for this PR to directly show a message to the user, so console.log is fine here (maybe use console.error to print to the console in red).

One thing that could change is the messaging. Error getting documents isn't very helpful to users. Maybe something like Error getting your shopping list?


// fetch the latest shopping list from the database and save to state
const fetchList = token => {
var db = firebase.firestore();

This comment has been minimized.

Copy link
@stevelikesmusic

stevelikesmusic Jan 19, 2020

Collaborator

I generally never use var anymore. Is the reason between choosing var vs let? Why not const?

This comment has been minimized.

Copy link
@prophen

prophen Jan 19, 2020

Author Contributor

I copied from the firebase docs and forgot to change it

import firebase from 'firebase/app';
import normalizeName from './lib/normalizeName';

const ListContext = React.createContext();

This comment has been minimized.

Copy link
@stevelikesmusic

stevelikesmusic Jan 19, 2020

Collaborator

A convenient way to expose your Context is via a hook. Then you can pass additional arguments in:

export const useShoppingList = useContext(ListContext);

Then importing the Context becomes as simple as a single import.

function SomeComponent(props) {
  const { list, fetchList } = useShoppingList();
}

This comment has been minimized.

Copy link
@prophen

prophen Jan 19, 2020

Author Contributor

I didn't quite understand how to do this, so I left it as is. I'll definitely read up on this soon.

This comment has been minimized.

Copy link
@stevelikesmusic

stevelikesmusic Jan 21, 2020

Collaborator

Sounds good @prophen! If you still have questions after digging in, I'm more than happy to pair up

@@ -11,7 +10,6 @@ export default function AppRouter() {
<Switch>
<Route exact path="/" component={HomePage} />
<Route path="/add-item" component={AddItem} />
<Route path="/new-list" component={NewList} />

This comment has been minimized.

Copy link
@stevelikesmusic

stevelikesmusic Jan 19, 2020

Collaborator

Was the intent here to auto-create a new list token if we don't have one?

This comment has been minimized.

Copy link
@prophen

prophen Jan 19, 2020

Author Contributor

It was leftover from when an earlier story when we tried starting a new list in a new component. The new list component was never used and we forgot to remove it.

@@ -0,0 +1,13 @@
//normalized function
const normalizeName = name => {

This comment has been minimized.

Copy link
@stevelikesmusic

stevelikesmusic Jan 19, 2020

Collaborator

What would you think about a regex here with String.prototype.replace?

How might you normalize the name without using a loop?

This comment has been minimized.

Copy link
@prophen

prophen Jan 19, 2020

Author Contributor

Made the change to regex here.

mikeramz86 and others added 6 commits Jan 19, 2020
Nikema Prophet
I implemented most of the changes Steve suggested. I removed the setTimeout and now the function checks for duplicates as you type.

Does this break UX? I tried entering apple then apple pie. The app will warn that apple is on the list but if you keep typing the warning goes away.
Nikema Prophet
Nikema Prophet
@prophen prophen requested a review from segdeha Jan 19, 2020
@prophen

This comment has been minimized.

Copy link
Contributor Author

prophen commented Jan 19, 2020

I tried to implement the suggestions @segdeha and @stevelikesmusic made in addition to the work that @mikeramz86 did earlier. I think I changed enough that it could use another mentor review.

@@ -1,45 +1,57 @@
import React, { useState, useEffect } from 'react';
import { withFirestore } from 'react-firestore';

This comment has been minimized.

Copy link
@prophen

prophen Jan 19, 2020

Author Contributor

The firebase stuff is happening in listContext.js now

Nikema Prophet added 2 commits Jan 19, 2020
Copy link
Contributor

segdeha left a comment

LGTM! Nice touch having it alert the user as they're typing!

Copy link
Contributor

mikeramz86 left a comment

Everything good to go!


return (
<ListContext.Provider
value={{

This comment has been minimized.

Copy link
@mikeramz86

mikeramz86 Jan 19, 2020

Contributor

@mikeramz86 review this message a little more

@mikeramz86 mikeramz86 merged commit b657dda into master Jan 19, 2020
5 checks passed
5 checks passed
Header rules - tcl-3-smart-shopping-list No header rules processed
Details
Pages changed - tcl-3-smart-shopping-list 6 new files uploaded
Details
Redirect rules - tcl-3-smart-shopping-list No redirect rules processed
Details
Mixed content - tcl-3-smart-shopping-list No mixed content detected
Details
netlify/tcl-3-smart-shopping-list/deploy-preview Deploy preview ready!
Details
@prophen prophen deleted the mr-np-check-for-duplicates branch Jan 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.