#50: Add functionality to undo deleting a product #94

biancadanforth · 2018-09-08T01:41:31Z

This patch makes it possible for the user to undelete a deleted product from their list of tracked products while the browserAction popup remains open. If the browserAction popup closes, any deleted products will be removed permanently.

Implementation notes:

Add the SET_DELETION_FLAG action which dispatches when the user clicks the ‘X’ button or Undo buton for a product.
- If X button: The product card is greyed out, cannot be clicked to open the product page, and an Undo button appears in the top right corner. The deletion flag for the product in storage is set to true.
- If Undo button: The product card is restored to its original view and behavior. The deletion flag for the product in storage is set to true.
Replace the REMOVE_PRODUCT action with the REMOVE_MARKED_PRODUCTS action which dispatches when the browserAction is closed.
- Any greyed out cards will be gone the next time the popup is opened.

In Nightly, when the REMOVE_MARKED_PRODUCTS action is dispatched, the error from Issue #60 is logged, though it does not affect the functionality of this patch.

Follow-up issues:

Add a loading GIF or screen on first render of browserAction popup #93 : Show a loading screen for the first render before the app's state has been loaded.
Update undo UI to match final design #116 Once this patch merges, file an issue for the final UI for this functionality with label design-pending.

This patch makes it possible for the user to undelete a deleted product to their list of tracked products while the browserAction popup remains open. If the browserAction popup closes, any deleted products will be removed permanently. Implementation notes: * Add the DELETE_PRODUCT action which dispatches when the user clicks the ‘X’ button for a product. * The product card is greyed out, cannot be clicked to open the product page, and an Undo button appears in the top right corner. * Add the UNDELETE_PRODUCT action which dispatches when the user clicks the ‘Undo’ button for a product. * The product card is restored to its original view and behavior. * Replace the REMOVE_PRODUCT action with the DELETE_MARKED_PRODUCTS action which dispatches when the browserAction is closed. * Any greyed out cards will be gone the next time the popup is opened. In Nightly, when the DELETE_MARKED_PRODUCTS action is dispatched, the error from Issue #60 is logged, though it does not affect the functionality of this patch. Follow-up issues: * #93: Show a loading screen for the first render before the app's state has been loaded.

biancadanforth · 2018-09-08T01:48:50Z

@Osmose Ready for your review. Thanks for all your help thus far; this is my first Redux patch ever!

Osmose

Nice work! The redux changes look sound. :D

Feel free to merge this after making the changes. Ping me if you want a second pass on the suggested Redux changes.

Osmose · 2018-09-14T21:48:09Z

src/browser_action/components/BrowserActionApp.jsx

@@ -41,6 +46,14 @@ export default class BrowserActionApp extends React.Component {

  componentDidMount() {
    this.props.loadStateFromStorage();
+    window.addEventListener('unload', this.handleUnload);


We should also remove this listener in componentWillUnmount. It'll ensure in tests that we don't leave any leftover listeners.

Osmose · 2018-09-14T22:02:41Z

src/browser_action/components/ProductCard.jsx

+     * If true, show an undo icon in the top right corner.
+     * Will not be shown unless the close icon is clicked.
+     */
+    showUndo: pt.bool,


Hmmm, I wonder if the undo should just be a wrapper component over the card instead of an extra option? Technically one could set both showClose and showUndo to true, and it's ambiguous what the behavior is in that case.

At the same time, depending on the new UX design for the Undo state, maybe we won't even need to show a product card without a close button, or below the undo state? It might be worth deferring to that decision and just landing this as is.

Yeah I'd say that change could go into the follow-up issue to update the Undo view with the final UI, which we are still waiting for.

Osmose · 2018-09-14T22:03:09Z

src/browser_action/components/ProductCard.jsx

        {latestPrice && (
          <div className="latest-price">
            {latestPrice.amount.toFormat('$0.00')}
+            {showUndo && (
+              <button className={showUndo ? 'undo-button' : 'undo-button hidden'} type="button" onClick={this.handleClickUndo}>


showUndo must be true for this to be displayed, so the false case will never be hit.

Osmose · 2018-09-14T22:03:57Z

src/browser_action/components/ProductCard.jsx

            {showClose && (
-              <button className="close-button" type="button" onClick={this.handleClickClose}>
+              <button className={showUndo ? 'close-button hidden' : 'close-button'} type="button" onClick={this.handleClickClose}>


Instead of hiding the button with CSS, add the showUndo check to the initial condition so that we don't render any HTML for it at all.

Osmose · 2018-09-14T22:04:26Z

src/browser_action/components/ProductCard.jsx

@@ -119,6 +145,7 @@ export default class ProductCard extends React.Component {
            {buttonText}
          </button>
        )}
+        <div className={showUndo ? 'opaque-overlay' : 'opaque-overlay hidden'} />


Instead of hiding it with CSS, we can just not render the overlay div at all if showUndo is false.

Osmose · 2018-09-14T22:06:03Z

src/state/products.js

@@ -21,6 +21,7 @@ export const productShape = pt.shape({
  title: pt.string.isRequired,
  url: pt.string.isRequired,
  image: pt.string.isRequired,
+  isDeleted: pt.bool.isRequired, // flag to include in returned list of products from store


Is this not the same for every attribute here? I don't quite understand the comment.

Osmose · 2018-09-14T22:06:42Z

src/state/products.js

-const REMOVE_PRODUCT = 'commerce/products/REMOVE_PRODUCT';
+const DELETE_PRODUCT = 'commerce/products/DELETE_PRODUCT';
+const UNDELETE_PRODUCT = 'commerce/products/UNDELETE_PRODUCT';
+const DELETE_MARKED_PRODUCTS = 'commerce/products/DELETE_MARKED_PRODUCTS';


Maybe use the word "remove" for actual removal, and "delete" for marking as ready to be removed? Otherwise "delete" has two meanings.

Osmose · 2018-09-14T22:07:29Z

src/state/products.js

+          if (product.id === action.productId) {
+            const flaggedProduct = {...product};
+            flaggedProduct.isDeleted = true;
+            return flaggedProduct;


These lines can be simplified a tiny bit:

return {...product, isDeleted: true};

Osmose · 2018-09-14T22:08:37Z

src/state/products.js

@@ -40,7 +41,9 @@ export const extractedProductShape = pt.shape({
 // Actions

 export const ADD_PRODUCT = 'commerce/products/ADD_PRODUCT'; // Used by price duck
-const REMOVE_PRODUCT = 'commerce/products/REMOVE_PRODUCT';
+const DELETE_PRODUCT = 'commerce/products/DELETE_PRODUCT';
+const UNDELETE_PRODUCT = 'commerce/products/UNDELETE_PRODUCT';


These two can be collapsed into a single action: SET_DELETION_FLAG. Then the action you dispatch can contain the value of the flag (true or false), rather than having an action for setting it to true and another for setting it to false.

Osmose · 2018-09-14T22:09:26Z

src/state/products.js

+
+/**
+ * Remove all products marked as deleted from the store.
+ * @param {string} productId Unique ID for the product to remove.


This does not take any parameters.

* Rename 'deleteMarkedProducts' to 'removeMarkedProducts'. * Remove 'unload' event listener on 'componentWillUnmount' in BrowserActionApp.jsx. * Simplify some of the JSX and CSS for the ProductCard component. * Consolidate the 'DELETE_PRODUCT' and 'UNDELETE_PRODUCT' Redux actions into a single SET_DELETION_FLAG action.

biancadanforth requested a review from Osmose September 8, 2018 01:41

Osmose approved these changes Sep 14, 2018

View reviewed changes

biancadanforth mentioned this pull request Sep 17, 2018

Update undo UI to match final design #116

Closed

1 task

biancadanforth force-pushed the 50-undo branch from 0ec6eba to f08ba67 Compare September 17, 2018 19:29

biancadanforth merged commit b5a1b2c into master Sep 17, 2018

biancadanforth deleted the 50-undo branch September 17, 2018 19:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#50: Add functionality to undo deleting a product #94

#50: Add functionality to undo deleting a product #94

biancadanforth commented Sep 8, 2018 •

edited

Loading

biancadanforth commented Sep 8, 2018

Osmose left a comment

Osmose Sep 14, 2018

Osmose Sep 14, 2018

biancadanforth Sep 17, 2018

Osmose Sep 14, 2018

Osmose Sep 14, 2018

Osmose Sep 14, 2018

Osmose Sep 14, 2018

Osmose Sep 14, 2018

Osmose Sep 14, 2018

Osmose Sep 14, 2018

Osmose Sep 14, 2018

#50: Add functionality to undo deleting a product #94

#50: Add functionality to undo deleting a product #94

Conversation

biancadanforth commented Sep 8, 2018 • edited Loading

biancadanforth commented Sep 8, 2018

Osmose left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

biancadanforth commented Sep 8, 2018 •

edited

Loading