Add Accordion component #26

Osmose · 2018-07-16T21:39:53Z

This adds an accordion component for showing multiple sections of content in the sidebar in a vertical stack. Only one page can be viewed at a time. There's two sections right now:

Collections, which is a placeholder for the collections view for Prototype saving products to collections #23.
Current Page, which shows the product detected on the current page. This section is not shown if no product is detected.

This also includes a bunch of baseline work for writing a React-based UI:

Adds aliases for doing absolute imports, which are slightly more flexible than relative imports and are more explicit.
Adds class properties to make defining propTypes and defaultProps cleaner on class-based components.
Adds legacy decorators so we can use autobind-decorator to handle automatic binding of methods on React components.
Adds css-loader and style-loader so that Webpack can handle importing CSS files (it automatically adds them to <head> via <style> tags.
Add stylelint to lint the newly-added CSS files.

There's also some minor fixes:

Refactors the props on the Sidebar component to use a single product object.
Refactors utils.js to export multiple functions directly instead of a single object.
Disables some eslint rules I'm not a huge fan of. In particular, while the a11y rules are useful and we should enable them before shipping, they're not helpful during the prototyping phase where we don't know what the final UX will be yet.

This is split into 3 commits, and reviewing it per-commit may be easier than trying to review everything in one big chunk.

biancadanforth

In general, this is super cool and modern! I feel like such a React dinosaur.

I don't see any major problems. One potential issue is the plugin order for babel plugins in .eslintrc.json.

I asked a LOT of questions and follow ups, and made some nit observations/suggestions, some of which you may consider for follow up issues. Very interested in your thoughts.

biancadanforth · 2018-07-17T01:30:38Z

package.json

@@ -19,6 +19,7 @@
    "copy-webpack-plugin": "4.5.2",
    "eslint": "4.19.1",
    "eslint-config-airbnb": "17.0.0",
+    "eslint-import-resolver-webpack": "^0.10.1",


Wow the docs for this are not clear. What does this allow us to do?

Looks like you created an alias called commerce for the absolute path to the ./src directory? Why is this an eslint module?

The eslint module allows eslint rules on imports to correctly understand the commerce alias. Without this, eslint would complain that commerce is not an installed package (because it's really an alias to our source directory to allow for absolute imports).

biancadanforth · 2018-07-17T01:32:10Z

src/product_info.js

@@ -2,7 +2,7 @@
 * License, v. 2.0. If a copy of the MPL was not distributed with this
 * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

-import utils from './utils';
+import {retry} from './utils';


Why does this file use './utils', where background.js uses 'commerce/utils'?

biancadanforth · 2018-07-17T01:34:20Z

src/sidebar/index.jsx

+  }
+}
+Sidebar.propTypes = {
+  product: pt.shape({


Cool; much easier than defining an entire schema in a separate file.

biancadanforth · 2018-07-17T01:38:03Z

src/sidebar/index.jsx

+import ReactDOM from 'react-dom';
+import pt from 'prop-types';
+
+const PRODUCT_KEYS = ['title', 'image', 'price'];


In the spirit of DRY-er code, it would be nice if we didn't have to declare this multiple times in different files... what about having a config.js(on) that's imported here and in background.js?

I imagine these keys will grow to a larger number, and there may be other config info that we need to share across multiple contexts.

I was thinking about that... the idea of a "Product" and the attributes of it are an important model, but I'm not yet sure how to express that. In particular, is title+image+price really the thing that determines a valid product? It depends on our featureset.

I could see us writing a class for Products that has these and other attributes, and does validation as well as serialization, and other things. But that will probably wait until we have a clear featureset. Until then, I think we should just leave this as is.

biancadanforth · 2018-07-17T01:54:48Z

src/sidebar/index.jsx

+  image: url.searchParams.get('image'),
+  price: url.searchParams.get('price'),
+};
+const isValidProduct = PRODUCT_KEYS.map(key => product[key]).every(val => val);


This looks really similar to hasKeys in utils.js. Could this make use of that method?

It's subtly different; hasKeys checks that the keys are present in product, but this checks that the keys actually have truthy values. The way we define product above means the keys are always present on the object, but might be an empty string or null.

I think we'll replcae this code with a better product model at some point anyway, so improving it isn't really worth it right now.

biancadanforth · 2018-07-17T17:17:23Z

src/sidebar/components/Accordion.jsx

+ *
+ * @example
+ *   <Accordion>
+ *     <Accordion.Section title="Section 1" initial>Section 1 Content</Accordion.Section>


Shouldn't this be <AccordionSection ...?

AccordionSection isn't exported directly, it's available as a static property on the parent component. I learned this pattern from ant-design and prefer it for UI patterns that require multiple components, because it lets you import just the default export Accordion and still have access to the child components that are also necessary.

biancadanforth · 2018-07-17T17:26:46Z

src/sidebar/index.jsx

+    product: undefined,
+  }
+
+  componentDidCatch() {


Didn't know about this method

Honestly neither did I, React was logging some stuff to the console about it so I added it to shut it up.

biancadanforth · 2018-07-17T17:32:59Z

src/sidebar/components/Accordion.jsx

+    return (
+      <div className={`${className} accordion`}>
+        {
+          // toArray strips out null children, which we want.


When would we have null children?

I don't entirely know, actually. Maybe blank text nodes between children? In any case, they're there, and this was failing when I used React.Children.map directly, so I switched to this instead.

I think you can have null children here in the case we are not on a product page, and therefore are only showing one of the two Accordion.Sections?

biancadanforth · 2018-07-17T17:40:05Z

src/sidebar/components/Accordion.jsx

+              active: childIndex === this.state.activeChildIndex,
+              onClick: this.handleSectionClicked,
+              index: childIndex,
+            }),


Okay so let me see if I understand what is happening here:

Accordion could (and likely will) have multiple AccordionSection children. You are using the React.Children.Array method on this.props.children to iterate through each section and assign its props passed down from Accordion (other props, like title, children and initial are currently passed down from Sidebar).

You are cloning the AccordionSections, so that you can extend their this.props?

Yup. Accordion is responsible for tracking which section is visible, and sections handle their rendering based on their props so they can remain simple. The user provides some props, Accordion provides the rest.

biancadanforth · 2018-07-17T17:42:31Z

src/sidebar/index.jsx

-        <li>{product.price || 'Unknown'}</li>
-      </ul>
+      <Accordion className="sidebar-container">
+        {product && (


Same thing here as my earlier question on this line; What exactly is this doing?

I refer to my previous answer.

Builds off PR #26. * Adds Jest/Enzyme and the first part of a simple React component test, which can be executed via 'npm run test'. * Adds eslint rules for Jest * Adds new devDeps: * babel-jest: Compiles JS code using Babel in Jest tests * babel-preset-env: Compiles ES2015+ down to ES5. Currently this is so we can use 'import' in Node.js for the Jest tests. * enzyme: JS testing library for React * enzyme-adapter-react-16: Allows Enzyme to be compatible with React 16 * eslint-plugin-jest: Expose Jest-specific rules for use in ESLint * jest: Testing framework for JS including React apps * react-test-renderer: A peer dependency for enzyme-adapter-react-16. An experimental React renderer that can be used to render React components to pure JS objects, without depending on the DOM or a native mobile environment. * regenerator-runtime * Adds a 'commerce' alias for Jest tests in 'package.json' for React components to match the 'commerce' alias used in Webpack. * Adds static asset mocks for Jest tests * Export both 'Accordion' and 'AccordionSection' classes from 'Accordion.jsx' rather than just 'Accordion', so that 'AccordionSection' can also be used in Jest tests. Questions: * How to get around the CSS import in Accordion.jsx? I currently have it commented out. * Is it possible for me to webpack the './test' directory? Would I want to? What might that look like? * How can I render the original '<Accordion />', instead of explicitly passing it two '<AccordionSection />' elements in the test? 'Accordion.jsx' renders two AccordionSections already with their own 'this.props.children'. * How can I fix the prop-types errors for required props? Do I just have to 'monkey patch' these components and pass dummy props into the components in the test? * Should I use 'babel-preset-env', so that I can use 'import' in the test files? These tests run in Node.js, and currently, Node.js v8 still does not support 'import'. * Jest docs (https://jestjs.io/docs/en/webpack) suggest using mocks for static assets. I followed their Webpack instrucions for this, but I'm not sure I follow what the mock files are actually for/doing.

Builds off PR #26. * Adds Jest/Enzyme and the first part of a simple React component test, which can be executed via 'npm run test'. * Adds eslint rules for Jest * Adds new devDeps: * babel-jest: Compiles JS code using Babel in Jest tests * babel-preset-env: Compiles ES2015+ down to ES5. Currently this is so we can use 'import' in Node.js for the Jest tests. * enzyme: JS testing library for React * enzyme-adapter-react-16: Allows Enzyme to be compatible with React 16 * eslint-plugin-jest: Expose Jest-specific rules for use in ESLint * jest: Testing framework for JS including React apps * react-test-renderer: A peer dependency for enzyme-adapter-react-16. An experimental React renderer that can be used to render React components to pure JS objects, without depending on the DOM or a native mobile environment. * regenerator-runtime: Compiles generators/yield from ES2015 to ES5. Suggested by Jest docs for using Babel. * Adds a 'commerce' alias for Jest tests in 'package.json' for React components to match the 'commerce' alias used in Webpack. * Adds static asset mocks for Jest tests * Export both 'Accordion' and 'AccordionSection' classes from 'Accordion.jsx' rather than just 'Accordion', so that 'AccordionSection' can also be used in Jest tests. Questions: * How to get around the CSS import in Accordion.jsx? I currently have it commented out. * Is it possible for me to webpack the './test' directory? Would I want to? What might that look like? * How can I render the original '<Accordion />', instead of explicitly passing it two '<AccordionSection />' elements in the test? 'Accordion.jsx' renders two AccordionSections already with their own 'this.props.children'. * How can I fix the prop-types errors for required props? Do I just have to 'monkey patch' these components and pass dummy props into the components in the test? * Should I use 'babel-preset-env', so that I can use 'import' in the test files? These tests run in Node.js, and currently, Node.js v8 still does not support 'import'. * Jest docs (https://jestjs.io/docs/en/webpack) suggest using mocks for static assets. I followed their Webpack instrucions for this, but I'm not sure I follow what the mock files are actually for/doing. * Do we need the 'regenerator-runtime' package? How can we be sure? I assume this is similar to 'babel-preset-env' in that it might be needed for Node.js, but it is not needed for Firefox.

Builds off PR #26. * Adds Jest/Enzyme and the first part of a simple React component test, which can be executed via 'npm run test'. * Adds eslint rules for Jest * Adds new devDeps: * babel-jest: Compiles JS code using Babel in Jest tests; this means Babel is run on any files loaded by Jest (and these files are transformed based on .babelrc). * babel-preset-env: Compiles ES2015+ down to ES5. Currently this is so we can use 'import' in Node.js for the Jest tests (Jest runs in Node); we add additional rules in .babelrc so that we only perform Babel transforms that are needed for the current Node environment, rather than transforming everything to ES5, which is not needed for Firefox. * enzyme: JS testing library for React * enzyme-adapter-react-16: Allows Enzyme to be compatible with React 16 * eslint-plugin-jest: Expose Jest-specific rules for use in ESLint * jest: Testing framework for JS including React apps * react-test-renderer: A peer dependency for enzyme-adapter-react-16. An experimental React renderer that can be used to render React components to pure JS objects, without depending on the DOM or a native mobile environment. * Adds a 'commerce' alias for Jest tests in 'package.json' for React components to match the 'commerce' alias used in Webpack. * Adds a style mock for Jest tests, so that we don't fail when we 'import' a stylesheet in a JSX component (this stylesheet 'import' statement is transformed by Webpack via the 'style-loader' and 'css-loader', but we are testing against source files currently) * Export both 'Accordion' and 'AccordionSection' classes from 'Accordion.jsx' rather than just 'Accordion', so that 'AccordionSection' can also be used in Jest tests. Limitations of using Jest: * Jest runs in Node. Node's JS implementation is different (how different?) from Firefox's JS implementation. * Jest only allows us to perform unit tests on React components. For integration testing, we will still need a solution that lets us interact with the Firefox browser (ex: Mochitests, Marionette). Questions: * How can I render the original '<Accordion />', instead of explicitly passing it two '<AccordionSection />' elements in the test? 'Accordion.jsx' renders two AccordionSections already with their own 'this.props.children'. * How can I fix the prop-types errors for required props? Do I just have to 'monkey patch' these components and pass dummy props into the components in the test?

An alias helps avoid relative import paths, which are more brittle than absolute imports. Also refactors utils.js to directly export functions instead of defining an object. Also refactors the sidebar to use a single product object as a prop.

…ent.

Osmose requested a review from biancadanforth July 16, 2018 21:39

biancadanforth approved these changes Jul 17, 2018

View reviewed changes

Osmose force-pushed the collections branch from 7c2528d to 4d31c1d Compare July 20, 2018 20:14

Michael Kelly added 3 commits July 20, 2018 13:14

Use alias for imports and refactor sidebar.

217f6f2

An alias helps avoid relative import paths, which are more brittle than absolute imports. Also refactors utils.js to directly export functions instead of defining an object. Also refactors the sidebar to use a single product object as a prop.

#23: Add Accordion component for displaying multiple sections of cont…

ed7b865

…ent.

Add styling and configure webpack to include CSS, and add stylelint.

da45486

Osmose force-pushed the collections branch from 4d31c1d to da45486 Compare July 20, 2018 20:14

Osmose merged commit cadba26 into master Jul 20, 2018

Osmose deleted the collections branch July 20, 2018 20:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Accordion component #26

Add Accordion component #26

Osmose commented Jul 16, 2018

biancadanforth left a comment

biancadanforth Jul 17, 2018

Osmose Jul 20, 2018

biancadanforth Jul 17, 2018

Osmose Jul 20, 2018

biancadanforth Jul 17, 2018

biancadanforth Jul 17, 2018

Osmose Jul 20, 2018 •

edited

biancadanforth Jul 17, 2018

Osmose Jul 20, 2018

biancadanforth Jul 17, 2018

Osmose Jul 20, 2018 •

edited

biancadanforth Jul 17, 2018

Osmose Jul 20, 2018

biancadanforth Jul 17, 2018

Osmose Jul 20, 2018

biancadanforth Jul 24, 2018

biancadanforth Jul 17, 2018

Osmose Jul 20, 2018

biancadanforth Jul 17, 2018

Osmose Jul 20, 2018

Add Accordion component #26

Add Accordion component #26

Conversation

Osmose commented Jul 16, 2018

biancadanforth 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

Osmose Jul 20, 2018 • edited

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

Osmose Jul 20, 2018 • edited

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

Osmose Jul 20, 2018 •

edited

Osmose Jul 20, 2018 •

edited