Skip to content

Conversation

@ryami333
Copy link
Contributor

@ryami333 ryami333 commented Jan 31, 2019

A big re-jig of the demo page and a couple of little updates to the getting-started example boilerplate.

Notes:

  • Fancy styles have been updated to put the arrow on the left rather than the right. They were dependent on a block-level descendant of AccordionItemHeading, which as we now know, is not permitted (only "phrasing content" is allowed inside a button).
  • Similarly, all heading tags nested inside AccordionItemHeading have been removed in both the demo and the README.
  • Removed the react react-dom from the install line in the README. It's a bit redundant - if people don't already have React installed then they're in the wrong place.
  • Used 'placeholder' content for accordion items on the demo page. It was a bit of a confusing mess to have content copied from the README in there, and a maintenance risk too.
  • Remove examples from the demo page which weren't strictly supported and reflected in the README. Moving away from supporting users' custom/bespoke solutions.
  • Made the demo app itself much more DRY. Abstracted away re-used parts (like the AccordionItems and the Arrow).

screenshot 2019-02-01 at 12 38 26 pm

@ryami333
Copy link
Contributor Author

ryami333 commented Feb 1, 2019

There's still a TODO on here, and the render-prop one will need updating too. But no dramas - only merging to a WIP branch.

</h3>
</AccordionItemHeading>
<AccordionItemPanel>
<Accordion allowMultipleExpanded={true}>
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure, was it valuable to show that you can put accordions inside accordions?

I'd also make sure that we still have some demo content that has a link or two inside the panel so we can demonstrate that tabbing to those will still work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I would consider re-instating the nested one. My thinking was that this demo could afford to be a lot more minimal, and that by showing too many different patterns it was becoming overwhelming.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, good idea re simplifying.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Links are a good idea... I'll whack some of those in there.

Copy link
Contributor Author

@ryami333 ryami333 Feb 1, 2019

Choose a reason for hiding this comment

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

Links there now @samanthaksanders. Going to merge now, and will revisit the nested accordion when I come back to do the 'TODOs'

@coveralls
Copy link

coveralls commented Feb 1, 2019

Coverage Status

Coverage remained the same at 98.99% when pulling 6811452 on chore/remove-redundant-demos into 7a05caf on next.

@ryami333 ryami333 merged commit 9d15b0e into next Feb 1, 2019
@ryami333 ryami333 deleted the chore/remove-redundant-demos branch February 1, 2019 02:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants