Delete shopping item

[connietran-dev]

Description

We've added the ability to delete a Shopping Item by adding a "Delete" button next to each item. A modal appears if you attempt to delete an item asking you to confirm that you'd like to delete it. If you select to delete the item, it removes the item from Firestore, and there is a handleCloseModal to close the modal.

We've also tried to make this modal accessible by allowing you to navigate to it and use it with your keyboard. We've used the following as resources for developing accessible modals:

• Aria practices: https://www.w3.org/TR/wai-aria-practices-1.1/examples/dialog-modal/dialog.html
• Keyboard navigation: https://tinloof.com/blog/how-to-create-an-accessible-react-modal/

Related Issue

Closes #11 : As a user, I want to be able to delete items from my shopping list so that my list isn’t cluttered with items I don’t want to buy in the future.

Acceptance Criteria

• User is able to delete an item from the shopping list
• Before deleting, prompt the user to confirm that they really want to delete the item to prevent accidental deletions
• Deletion should cause the associated record(s) in the database to be deleted

Type of Changes

	Type
	🐛 Bug fix
✓	✨ New feature
	🔨 Refactoring
	💯 Add tests
	🔗 Update dependencies
	📜 Docs

Updates

Before

After

Testing Steps / QA Criteria

Functionality:

1. Click "Delete" next to an item.
2. In the modal, clicking "No, Cancel" will close the modal.
3. In the modal, clicking "Yes, Delete" will remove the item from the Shopping List.
4. The item is no longer in Firestore.

Accessibility steps:

1. You should be able to use your keyboard to navigate and select "Delete".
2. When the modal opens, the focus will be on "No, Cancel".
3. You can tab in the modal which will navigate you to either "No, Cancel" or "Yes, Delete".
4. You can use Enter to select your option.
5. You can escape to close the modal.

[github-actions]

Visit the preview URL for this PR (updated for commit 4481d7e):

https://tcl-26-shopping-list--pr44-ct-sm-delete-items-40a8eynk.web.app

_{(expires Sun, 29 Aug 2021 01:12:55 GMT)}

_{🔥 via Firebase Hosting GitHub Action 🌎}

[nick-zanetti]

Really impressive stuff here, everything in this useEffect is new to me conceptually but you really commented it well and made it easier to understand. I am curious though, are all the rules around Shift + Tab and different combinations necessary when we only have two possible elements to navigate to (the two buttons)? What is the default behavior if we don't have the rules laid out in handleKeyEvents ?

UPDATE: Reading the article you linked I think I might have found the answer. Without these rules, would a user basically be able to leave the modal through Tab and/or Shift + Tab ?

[connietran-dev]

Yes! If you comment this code out and use a keyboard to open the modal, if you start tabbing or shift/tabbing into the modal, the keyboard focus will start leaving the modal into the shopping list behind it.

[nick-zanetti]

Passing this down so many levels makes me uncomfortable, but since the app if functionally finished at this point, I think it's fine.

[connietran-dev]

This was definitely a struggle! We originally had most of this modal code right at ShoppingList. But because we add an aria-hidden=true to hide the App when the modal is open, we had to move everything up to App.js. We can definitely consider refactoring to use React Context in the future so we don't have to prop drill something like this so many levels down.

[anderswift]

@nick-zanetti Maybe we can make an issue for refactoring some things into React context?

[nick-zanetti]

@anderswift @connietran-dev I actually think that would be more work than it's worth, since the app is now essentially done functionally.

[anderswift]

@nick-zanetti I don't think it's necessary, but might be a good opportunity to practice with React context if anyone has some time and wants to give it a try. Refactoring can be a really great learning experience.

[connietran-dev]

I've logged #45 as a future enhancement/issue we could take on 😃

[nick-zanetti]

Great job! Even though modals have a lot of accessibility concerns, you seem to have handled all of them really well. All the AC look fulfilled to me.

I am a little curious as to why you ultimately went with the modal vs. a confirm dialog, since the latter seems like it might have been less work? I assume we'll discuss on Sunday.

[anderswift]

I love that you decided to take on the tricky task of building an accessible modal from scratch! The screen readers I tested did really well with the way you structured the UI, reading out helpful text at all times—good job figuring out the aria attributes!

It would be great for both screen reader and keyboard users if we could find a way to add that one missing detail: returning focus to the right place in the list after the modal closes. I also noticed that extra event handlers are accumulating as the modal opens/closes, so made a couple suggestions for a possible fix. Happy to help work through any of that, and maybe we can get some mentor feedback as well.

[anderswift]

I like that you handled this with classes vs. something like {showModal && <Modal />}, because we'll be able to easily add in open/close animation down the line!

We can always change this later, but wanted to mention one idea here. If we use more semantic classes like .dialog and .dialog_open, it would give us more flexibility later to easily change the styles without having to change the class names in the component to make sense. (As an example, when we make everything pretty in a future sprint, we might want to add in CSS transition styles so the modal fades in and out instead of just display:block/none.)

[yenly]

+1 on meaningful naming.

[connietran-dev]

This is a great suggestion. We've made a new commit to rename these CSS classes to .dialog-open and .dialog-close so that they're related to the dialog and we can keep adding CSS to them. We've moved them into Modal.js. For now, we didn't add just a .dialog class but we certainly can in the future

[anderswift]

Tested with a screen reader and it works great!

[abowler2]

While this does work to hide the element and as such keeps the screen reader from hitting it, I would caution against this approach as the component still renders and any logic within it will run.

As an example, I added a console.log within the Modal component and as you can see below that fired and logged to the console with the hidden attribute vs conditionally rendering the component using showModal && <Modal />

aria-hidden	conditional render
aria-hidden.mov	conditional-render.mov

[yenly]

This is an interesting approach. I'm not sure what the right way to approach this as well. Will have to research it.

[anderswift]

@abowler2 That is true! Hmm. For modals, drop down menus, etc. I've always done them this same way, having them render (or just be included in markup if vanilla JS) but shown/hidden with a CSS class, so later I could use CSS transitions to animate them fading or sliding in and out of view. The bootcamp I attended taught this method as well. Is this basically bad for performance in React? And is there a good alternate way you can still have an animation with the showModal && <Modal /> approach?

[anderswift]

@yenly I think it comes from this w3.org page. From what we could determine, the aria-modal attribute on the modal alone should take care of hiding things that are not the modal, but it is new so not fully supported everywhere. For full support, it's suggested to use aria-hidden="true" on "each element containing a portion of the inert layer" and that the modal must not be inside an aria-hidden layer. I found this general pattern pretty common:

<div aria-hidden="true">the whole rest of the page</div>
<div aria-modal="true">the active modal</div>

Though what we weren't certain of is what percentage of screen reader users might have legacy software that doesn't fully support aria-modal, therefore how necessary aria-hidden really is!

[connietran-dev]

Thank you all for the helpful input, especially April with the gifs (very helpful and clear what the issue is.) Is this something we can discuss Sunday if there is time?

[anderswift]

I noticed there is a compile warning about how handleKeyEvents is missing from the useEffect dependency array, and in trying to fix it I realized there are some problems with the event listeners getting added every time the modal is opened. (Because the handleKeyEvents function is recreated every time the component renders, the removeEventListener doesn't recognize each copy as the "same" function so, the listener is only added and never removed.)

One way we could fix this is by adding the tab/tab+shift handling functionality to onKeyDown event handlers on the two modal buttons instead of having a listener on the entire document. Then we could move the escape key stuff to App.js and wrap it in useCallback to ensure it is not being recreated on re-render.

Here's some code I tested for the tab/focus trap part which hopefully will help (I'll leave another comment about the escape key close):

Suggested change

	useEffect(() => {
	if (showModal) {
	// when modal opens, add eventListeners and put initial focus on "No, Cancel"
	document.addEventListener('keydown', handleKeyEvents);
	cancelRef.current.focus();
	} else {
	document.removeEventListener('keydown', handleKeyEvents);
	}
	}, [showModal]);
	const handleKeyEvents = (e) => {
	// close modal if user hits Escape (27)
	if (e.keyCode === 27) {
	handleModalClose();
	}
	// this keeps keyboard focus within modal. if user hits Tab (9)
	if (e.keyCode === 9) {
	// ...and NOT shift while on Delete, put focus on Cancel
	if (!e.shiftKey && document.activeElement === deleteRef.current) {
	cancelRef.current.focus();
	return e.preventDefault();
	}
	// ...and shift while on Cancel, put focus on Delete
	if (e.shiftKey && document.activeElement === cancelRef.current) {
	deleteRef.current.focus();
	return e.preventDefault();
	}
	}
	};
	return (
	<div className={`dialog-backdrop ${toggleModalClassName}`}>
	<div role="alertdialog" aria-modal="true" aria-labelledby="dialog_label">
	<h3 id="dialog_label">
	{`Are you sure you want to delete ${item.itemName}?`}
	</h3>
	<button type="button" onClick={handleModalClose} ref={cancelRef}>
	No, Cancel
	</button>
	<button
	type="button"
	onClick={deleteItem}
	aria-controls={`item-${item.id}`} // destructive delete controls shopping list item id
	ref={deleteRef}
	>
	Yes, Delete
	</button>
	</div>
	</div>
	);
	};
	useEffect(() => {
	if (showModal) cancelRef.current.focus();
	});
	const cancelButtonKeyDown = (e) => {
	// this keeps keyboard focus within modal
	if (e.keyCode === 9 && e.shiftKey) {
	// if user hits Tab (9) + Shift while on Cancel, put focus on Delete
	deleteRef.current.focus();
	return e.preventDefault();
	}
	};
	const deleteButtonKeyDown = (e) => {
	// this keeps keyboard focus within modal
	if (e.keyCode === 9 && !e.shiftKey) {
	// if tab (no shift) while on Delete, put focus on Cancel
	cancelRef.current.focus();
	return e.preventDefault();
	}
	};
	return (
	<div className={`dialog-backdrop ${toggleModalClassName}`}>
	<div role="alertdialog" aria-modal="true" aria-labelledby="dialog_label">
	<h3 id="dialog_label">
	{`Are you sure you want to delete ${item.itemName}?`}
	</h3>
	<button type="button" onClick={handleModalClose} ref={cancelRef} onKeyDown={cancelButtonKeyDown}>
	No, Cancel
	</button>
	<button
	type="button"
	onClick={deleteItem}
	aria-controls={`item-${item.id}`} // destructive delete controls shopping list item id
	ref={deleteRef}
	onKeyDown={deleteButtonKeyDown}
	>
	Yes, Delete
	</button>
	</div>
	</div>
	);
	};

[anderswift]

See my other comment on Modal.js about event listeners getting added too many times. I suggested there that one potential fix would be to split the handleKeyEvents function up, connecting the tab/tab+shift handling to onKeyDown events on the buttons, and then moving the escape key stuff into a separate function here in App.js. Why move it to App.js? So we can wrap it in a useCallback with no dependencies and ensure we only have one version of the function. (In Modal.js handleModalClose would be a dependency because it's passed down as a prop.)

I put sample code for the first part in my other comment, and here's the escape key portion I tested if you want to try it out. (Note that you would need to import useCallback at the top of this file, so import { useState, useEffect, useCallback } from 'react';.)

  // close modal if user hits Escape (27)
  const closeModalOnEsc = useCallback((e) => {
    if (e.keyCode === 27) handleModalClose();
  }, []);

  useEffect(() => {
    if(showModal) {
      document.addEventListener('keydown', closeModalOnEsc);
    } else {
      document.removeEventListener('keydown', closeModalOnEsc);
    }
  }, [showModal, closeModalOnEsc]);

Optional addition: we could even put in the "close when backdrop clicked" feature we didn't have time to finish up pretty easily by expanding on the above code like so...

  // close modal if user hits Escape (27)
  const closeModalOnEsc = useCallback((e) => {
    if (e.keyCode === 27) handleModalClose();
  }, []);

  // close modal if user hits Escape (27)
  const closeModalOnClickAway = useCallback((e) => {
    if (e.target.classList.contains('dialog-backdrop')) handleModalClose();
  }, []);

  // when modal opens/closes, add/remove event listeners
  useEffect(() => {
    if(showModal) {
      document.addEventListener('keydown', closeModalOnEsc);
      document.addEventListener('click', closeModalOnClickAway);
    } else {
      document.removeEventListener('keydown', closeModalOnEsc);
      document.removeEventListener('click', closeModalOnClickAway);
    }
  }, [showModal, closeModalOnEsc, closeModalOnClickAway]);

[anderswift]

Screen reader reads this out perfectly! Very good UX in JAWS and NVDA. 👍

[anderswift]

@nick-zanetti Maybe we can make an issue for refactoring some things into React context?

[yenly]

👏🏽 I'm impressed with your choice in creating a custom dialog component and making it keyboard navigation-friendly. 🚀

[yenly]

+1 on meaningful naming.

[yenly]

This is an interesting approach to save the item object for deletion in App state before doing so.

[Cjstoney]

This is more of a question for clarity on my end, but why is the ID "separate from Firestore"? It seems we need to associate the id with an item in the Firestore in order to delete it, but it is not coming from the item object itself?

[anderswift]

@Cjstoney The structure of the data returned from Firestore has the document id "separate" from the field data (doc.id has the id and doc.data() returns an object with the other fields and values). We originally didn't do anything to combine these values and just used item and itemId to hold these different bits from Firestore.

This sprint Nick and I decided to reorganize the data returned from Firestore before passing it down through the app, so that we'd have a simpler, cleaner array of items with item.id and some other helpful properties we needed. We coordinated with Connie & Sheila about this coming change, and that may have informed this line if I'm remembering correctly, since setItemToDelete({...item, id: itemId}) would work with their current code but be really easy to simplify to setItemToDelete(item) once we merge our branches together and no longer have a separate itemId prop.

[yenly]

This is an interesting approach. I'm not sure what the right way to approach this as well. Will have to research it.

[abowler2]

To second what @yenly said, really nice work taking on the modal for this issue.

[Cjstoney]

This works really well and is quite impressive, especially considering it was built from scratch and works with keyboard navigation. Good work!

[Cjstoney]

This is more of a question for clarity on my end, but why is the ID "separate from Firestore"? It seems we need to associate the id with an item in the Firestore in order to delete it, but it is not coming from the item object itself?