React: Add Standard Accordion Components to Mayflower React #85

avrilpearl · 2018-07-23T20:00:32Z

Description: This PR includes the addition of 4 new components that together create a standard accordion feature in Mayflower React. The design is pulled from Mayflower PL but were refactored in order to generate these new components. Also, in order to enable animations, this PR installs the react-transition-group library.

Question to Validate with React Team --> Is the approach to handling children in both accordion item and accordion wrapper appropriate here? Is the approach to animations here appropriate? Do we want to pull animation and focus scss in accordion item out to a higher level in the react library so it applies to all relevant components?

External Review Steps:

npm install
Go to @atoms/icons/SvgCircleChevron and confirm that the icon exist and matches mayflower PL
Go to @molecules/AccordionItem and confirm that a single accordion item is displayed, matching the appearance of a single item in Mayflower PL
Go to @organisms/AccordionWrapper and confirm that multiple accordion items are grouped and follow the same behavior as Mayflower PL

Updated Gifs

This reverts commit 7b1bc7b.

clairesunstudio · 2018-08-08T18:41:25Z

react/src/components/molecules/AccordionItem/index.js

+        <Collapse in={this.state.open} dimension="height">
+          <div className="ma__accordion-content__body">
+            { React.Children.map(this.props.children, (child) => {
+              if (allowedChildren.includes(child.type.name)) {


maybe should also check for React.isValidElement(child)) before child.type.name

this could also be refactored into a utility function?

added that to the wrapper - we can remove the child check if that is prefered here

clairesunstudio · 2018-08-08T18:42:02Z

react/src/components/molecules/AccordionItem/index.js

+        <Collapse in={this.state.open} dimension="height">
+          <div className="ma__accordion-content__body">
+            { React.Children.map(this.props.children, (child) => {
+              if (allowedChildren.includes(child.type.name)) {


this could also be refactored into a utility function?

clairesunstudio · 2018-08-08T18:43:25Z

react/src/components/molecules/AccordionItem/index.js

+  render() {
+    const buttonClasses = this.state.open ? 'ma__accordion-header__button is-open' : 'ma__accordion-header__button';
+    const accordionClasses = this.props.border ? 'ma__accordion-item' : 'ma__accordion-item__borderless';
+    const allowedChildren = ['Paragraph', 'OrderedList', 'UnorderedList', 'Heading', 'Table'];


where does this list come from? also see this in patternlab --> https://mayflower.digital.mass.gov/?p=molecules-contact-us-collapsed-with-more-link

we can remove any check for children type if that is a better approach here..

clairesunstudio · 2018-08-08T18:45:16Z

react/src/components/molecules/AccordionItem/index.js

+                {this.props.icon}
+              </div>
+            )}
+            <h2 className="ma__accordion-header__title">{this.props.title}</h2>


heading level should probably be passable

clairesunstudio · 2018-08-09T14:04:37Z

I think this is a good use of children to prevent prop drilling. I can see many organisms or higher components can be refactored to use children especially for those don't care what order of children are passed and don't need to carry any logic at the wrapper level. The react-transition-group can potentially solve all the animation problems we are running into, e.g. page scroll. For this specific case, since everything is purely css, maybe we could use ReactCSSTransitionGroup a higher level api to handle the animation. thoughts?

comments about css files were mistaken

…assgov/mayflower into react-add-accordion-to-mayflower-react

clairesunstudio · 2018-08-20T21:15:43Z

react/src/components/molecules/AccordionItem/style.scss

+    }
+
+    &__title {
+      @include ma-h5;


why import both h4 and h5 styles?

if you want to include the margin, I'd recommend only import h4 and manually set margin

i think i took this from a pl scss - but i will pull it out

clairesunstudio · 2018-08-20T21:31:03Z

Made some small changes to scss. The styles look good! There are a few warnings in AccordionItem.

This could be a follow up in design --> currently accordions don't have a hover state style, and for secondary accordion, it might not seem actionable.

pmallett-mc

see code comments

pmallett-mc · 2018-08-22T16:54:24Z

react/backstop/backstop.js

@@ -10,15 +11,27 @@ let viewports;

 // Scan for component names and set up viewports.
 const componentsPath = `${__dirname}/../src/components/`;
+components = listDirs(componentsPath)


I think we should keep the main backstop.js as free from distraction as possible (it's already got more than we probably want with my split of atoms/non-atoms)-- it shouldn't care about how to get the list of components, that's a storybook-specific thing.

Instead of looping over the results of the other call into fs here & doing more fs access, let's rename storyBookBackstop listDirs to something like listComponents & fold this logic in there.

pmallett-mc · 2018-08-22T16:56:19Z

react/backstop/backstop.js

    // Component directory names are capitalized.
    .filter((filePath) => (/^[A-Z]/.test(path.basename(filePath))))
    // Only test atoms with this backstop configuration.
    .filter((filePath) => (filePath.indexOf('/atoms/') > -1))
    // Skip table and media/Image, they need to be tested with larger viewports.
-    .filter((filePath) => ((filePath.indexOf('table') === -1) && (path.basename(filePath) !== 'Image')));
+    .filter((filePath) => ((filePath.indexOf('table') === -1) && (path.basename(filePath) !== 'Image')))
+    // Do not test components without stories;


Did this comment go with a line of code somewhere else?

pmallett-mc · 2018-08-22T16:59:31Z

react/src/components/animations/Collapse/index.js

+}
+
+function triggerBrowserReflow(node) {
+  node.offsetHeight; // eslint-disable-line no-unused-expressions


Is this doing anything?

yeah this causes the redraw so the animation can happen

pmallett-mc · 2018-08-22T17:06:22Z

react/src/components/molecules/CalloutLink/CalloutLink.stories.js

@@ -8,7 +8,7 @@ import CalloutLink from './index';
 import CalloutLinkDocs from './CalloutLink.md';
 import calloutLinkOptions from './CalloutLink.knobs.options';

-storiesOf('molecules/CalloutLink', module).addDecorator(withKnobs)


I think these changes resulted in stories being listed in two places

yeah this is so that backstop vr works correctly, it was getting the wrong stories before because of this nesting and that there is not a one to one relationship between src folder structure and storybook navigation - this was the simplest solution to solve this issue but we may want to refactor this in a followup

Could we do what we discussed about using the add.("[ComponentName]") in stories to create the reference url?

pmallett-mc · 2018-08-22T17:06:36Z

react/src/components/molecules/ImagePromo/ImagePromo.stories.js

@@ -34,12 +34,14 @@ const getCommonPropsWithKnobs = () => ({
  phone: null
 });

-storiesOf('molecules/ImagePromo', module).addDecorator(withKnobs)


see above comment

pmallett-mc · 2018-08-22T17:09:14Z

react/src/components/organisms/Footer/Footer.stories.js

@@ -13,7 +13,7 @@ import SocialLinksLiveData from '../../molecules/SocialLinks/SocialLinksLive.jso
 import stateSeal from '../../../../../assets/images/stateseal.png';


-storiesOf('organisms/Footer', module).addDecorator(withKnobs)
+storiesOf('organisms', module).addDecorator(withKnobs)


same issue as other example

clairesunstudio · 2018-08-22T18:28:38Z

react/backstop/backstop.js

@@ -1,4 +1,4 @@
-const listDirs = require('./listDirs');
+const listComponents = require('./listComponents');
 const storyBookBackstop = require('./storyBookBackstop');
 const path = require('path');
 const fs = require('fs');


this line can be removed

…ub.com/massgov/mayflower into react-add-accordion-to-mayflower-react

pmallett-mc · 2018-08-22T18:55:28Z

react/src/components/organisms/AccordionWrapper/index.js

+        return(
+          /* eslint-disable no-console */
+          console.log(`Warning! You cannot pass a ${child.type.name} child to AccordionWrapper`)
+          /* eslint-disable no-console */


I think this line was supposed to be eslint-enable

why should this be enable?

That or line 20 was maybe supposed to be eslint-disable-next-line instead of just disable

wip

c1fc5fa

avrilpearl added the WIP - Do not merge label Jul 23, 2018

avrilpearl added 12 commits July 23, 2018 16:52

refactor accordion item

6d16d67

revert css build and watch changes

5e556b5

add accordion wrapper doc

cd0fe23

fix lint errors

7b1bc7b

Revert "fix lint errors"

6edcc66

This reverts commit 7b1bc7b.

update collapse animation

cb82691

fix collapse lint error wip

3408499

fix lint error svgcirclechevron

88b903d

fix accordionitem lint errors

eabbd2f

fix accordion wrapper lint errors

21eebc6

fix lint warnings

abcd6f1

add credit note to collapse component

4c2c9fc

avrilpearl changed the title ~~[WIP] Adds Accordon Component to Mayflower React~~ React: Add Standard Accordion Components to Mayflower React Jul 24, 2018

avrilpearl requested a review from mrjmd July 24, 2018 13:29

avrilpearl added the internal review label Jul 24, 2018

avrilpearl requested review from smurrayatwork, pmallett-mc, clairesunstudio and michael-wojcik-mediacurrent July 24, 2018 13:29

avrilpearl added 2 commits July 24, 2018 09:33

add new components to export

6257259

add comment relating to children props of Accordion Components

b867066

avrilpearl mentioned this pull request Jul 24, 2018

create accordion in mayflower-react massgov/edit-mayflower-docs#5

Open

avrilpearl removed the WIP - Do not merge label Aug 8, 2018

clairesunstudio reviewed Aug 8, 2018

View reviewed changes

avrilpearl added 2 commits August 20, 2018 09:47

add secondary accordion style

b445289

add secondary accordion to mayflower-react

1ff066c

clairesunstudio and others added 4 commits August 20, 2018 17:06

update screenshot

7d20435

scss clean up

e046816

add changelog

4b4bf10

Merge branch 'react-add-accordion-to-mayflower-react' of github.com:m…

e05c691

…assgov/mayflower into react-add-accordion-to-mayflower-react

clairesunstudio reviewed Aug 20, 2018

View reviewed changes

avrilpearl added 11 commits August 21, 2018 09:41

move scss into asset level

9b0fd9f

move accordion wrapper scss to asset level

1c44f28

Merge branch 'develop' into react-add-accordion-to-mayflower-react

6a42513

allow any child to be passed to accordion item

a9a3d09

update accordion wrapper story

4bd77ca

add accordion references, do not run vr on animations

a9ec4e3

remove import

a88ef70

fix vr issues

ec3a81f

update changelog

9ace6d1

fix accordion item story

999103b

add classnames dependency, update accordion item classname logic

a162cbb

pmallett-mc suggested changes Aug 22, 2018

View reviewed changes

move list components out of react backstop

f45bc83

clairesunstudio reviewed Aug 22, 2018

View reviewed changes

clairesunstudio and others added 3 commits August 22, 2018 14:37

remove unuse const

19cb84a

remove fs from backstop

fabba8a

Merge branch 'react-add-accordion-to-mayflower-react' of https://gith…

5097ef4

…ub.com/massgov/mayflower into react-add-accordion-to-mayflower-react

clairesunstudio approved these changes Aug 22, 2018

View reviewed changes

pmallett-mc approved these changes Aug 22, 2018

View reviewed changes

pmallett-mc reviewed Aug 22, 2018

View reviewed changes

remove eslint disable

6d62db7

clairesunstudio merged commit 0fa00f8 into develop Aug 22, 2018

clairesunstudio deleted the react-add-accordion-to-mayflower-react branch August 22, 2018 19:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

React: Add Standard Accordion Components to Mayflower React #85

React: Add Standard Accordion Components to Mayflower React #85

avrilpearl commented Jul 23, 2018 •

edited

Loading

clairesunstudio Aug 8, 2018

clairesunstudio Aug 8, 2018

avrilpearl Aug 20, 2018

clairesunstudio Aug 8, 2018

clairesunstudio Aug 8, 2018

avrilpearl Aug 20, 2018

clairesunstudio Aug 8, 2018

clairesunstudio commented Aug 9, 2018

clairesunstudio Aug 20, 2018

clairesunstudio Aug 20, 2018

avrilpearl Aug 20, 2018

clairesunstudio commented Aug 20, 2018 •

edited

Loading

pmallett-mc left a comment

pmallett-mc Aug 22, 2018

pmallett-mc Aug 22, 2018

pmallett-mc Aug 22, 2018

avrilpearl Aug 22, 2018

pmallett-mc Aug 22, 2018

avrilpearl Aug 22, 2018

clairesunstudio Aug 22, 2018

pmallett-mc Aug 22, 2018

pmallett-mc Aug 22, 2018

clairesunstudio Aug 22, 2018

pmallett-mc Aug 22, 2018

clairesunstudio Aug 22, 2018

pmallett-mc Aug 22, 2018

React: Add Standard Accordion Components to Mayflower React #85

React: Add Standard Accordion Components to Mayflower React #85

Conversation

avrilpearl commented Jul 23, 2018 • edited Loading

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

clairesunstudio commented Aug 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clairesunstudio commented Aug 20, 2018 • edited Loading

pmallett-mc 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avrilpearl commented Jul 23, 2018 •

edited

Loading

clairesunstudio commented Aug 20, 2018 •

edited

Loading