Skip to content
This repository has been archived by the owner on Jul 10, 2020. It is now read-only.

Remove option for accordion behavior from expandables documentation #593

Merged
merged 6 commits into from
Feb 22, 2018

Conversation

marteki
Copy link
Member

@marteki marteki commented Feb 21, 2018

As shared in cfpb/capital-framework#536 and cfpb/capital-framework#665, we’ve removed the accordion option from our design patterns. This updates our documentation to match the implementation guidelines.

Removals

  • Remove the description of the (buggy, not used) accordion behavior for expandables

Review

Screenshots

Documentation before changes

screen shot 2018-02-21 at 2 31 34 pm

Documentation after changes

screen shot 2018-02-21 at 2 30 29 pm

Checklist

  • Changes are limited to a single goal (no scope creep)
  • Code can be automatically merged (no conflicts)
  • Code follows the standards laid out in the front end playbook
  • Passes all existing automated tests
  • New functions include new tests
  • New functions are documented (with a description, list of inputs, and expected output)
  • Placeholder code is flagged
  • Visually tested in supported browsers and devices
  • Project documentation has been updated (including the "Unreleased" section of the CHANGELOG)

As shared in cfpb/capital-framework#536 and
cfpb/capital-framework#665, we’ve removed the
accordion option from our design patterns. This updates our
documentation to match the implementation guidelines.
@marteki
Copy link
Member Author

marteki commented Feb 21, 2018

Whew! The build passed in Travis now. Needed to shift one of the dependencies and point it at a different location. Thanks to @Scotchester for his help on troubleshooting that.

package.json Outdated
@@ -32,6 +32,7 @@
"cf-pagination": "^4.2.2",
"cf-tables": "^4.2.1",
"cf-typography": "^4.6.0",
"grunt-contrib-uglify-es": "^3.3.0",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should replace grunt-contrib-uglify in the devDependencies list, and uglify-es can also be removed now.

Updated package ended up in `dependencies` instead of
`devDependencies`. Corrected that.
Copy link
Contributor

@Scotchester Scotchester left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is totally fine now, but just to ask the question: Should we remove the Interactions section entirely, now? It seems a bit superfluous without the Accordion content.

@marteki marteki mentioned this pull request Feb 21, 2018
9 tasks
@marteki
Copy link
Member Author

marteki commented Feb 21, 2018

@Scotchester Hmm. Maybe, if it's merged with the State section. I feel odd about having such an extensive Style section without explicitly calling out the Interactions, since it's an interactive web element.

Alright, I'll add that piece as a TODO to the list on our internal design patterns tracking page, and merging this.

@sebworks
Copy link

Hmmm, we are using the accordion on the OAH pages ( closing-disclosure / loan-estimate ) and I'm not in favor of removing them. The accordion allows you manage long content on mobile, without forcing the user to scroll or close other expandables.

@marteki
Copy link
Member Author

marteki commented Feb 21, 2018

@sebworks There's been a good bit of discussion about this already over the last several months, where designers and developers came to an understanding about the removal of the accordions. For more details and to catch up, see the summary at https://[GHE]/CFPB/hubcap/wiki/Design-pattern-additions-and-changes#expandable-accordions-removed.

The current plan was to adjust the expandable groups for the one known place where accordions are being used, but the Loan Estimate page was not previously called out as a place! Let's discuss it more internally.

(Somewhat related, and an issue you were previously tagged on for comment that might have gotten buried in your inbox: cfpb/capital-framework#536)

Copy link
Member

@ielerol ielerol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

@marteki
Copy link
Member Author

marteki commented Feb 22, 2018

Merging this, especially since the Travis work is needed for any other PRs to be merged successfully and #594 is waiting on it.

@marteki marteki merged commit fc4b553 into gh-pages Feb 22, 2018
@jimmynotjim jimmynotjim deleted the remove-expandable-accordions branch February 22, 2018 17:36
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants