breaking(Accordion): Refactor component

[layershifter]

BREAKING

This PR fully removes the children API from Accordion component. After this PR component will work in same manner as Menu.

Before

  <Accordion>
    <Accordion.Title>Title 1</Accordion.Title>
    <Accordion.Content>Content 1</Accordion.Content>
    <Accordion.Title>
      <div>Title 2</div>
    </Accordion.Title>
    <Accordion.Content>
      <div>Content 2</div>
    </Accordion.Content>
  </Accordion>

After

const panels = [
  { title: 'Title 1', content: 'Content 1' },
  { title: { content: <div>Title 2</div> }, content: { content: <div>Content 2</div> }},
]

<Accordion panels={panels}>

Please follow updated documentation for more examples.

---

We are removing magic functionality from our components (#1362, etc). The Accordion component is source of incompatible design.

I wan't to make serious refactor there, that will make breaking changes.

Fixes: #607
Addresses: #623

---

• Remove magic around children, none of our components doesn't implement it. If user wants to manage Accordion automatically he must use shorthands. Otherwise, user can use full markup, but he need to manage state himself. Example of this logic is Menu component.
• Standartize event handlers.
• Add Accordion.Panel subcomponent for easier management.
• Fully implement shorthands.
• Update docs, tests and typings.

@levithomason @jcarbo Thoughts?

[levithomason]

I agree with all these points. I'm not sure on the Panel subcomponent anymore, though. I dabbled with doing this for a bit and decided to think on it more.

The issue there is that a component must return a single root node (i.e. a single div), but, a Panel would need to return an array of Title/Content pairs:

<Accordion>
  <Panel title='The title' content='The content' />
</Accordion>

// renders

<Accordion>
  <Accordion.Title>The title</Accordion.Title>
  <Accordion.Content>The content</Accordion.Content>
</Accordion>

There are ways to accomplish this if we consider the Accordion.Panel as a pseudo element, however, that pattern only works if we can guarantee that the children will only ever be panels, which we cannot. We don't want to get into cases where users are mixing Panel pseudo children with regular children, since, using the pseudo child is the equivalent of using the panels prop, so children would not be allowed.

This has me wondering if we should just stick to the panels prop alone. Feedback would be great.

[layershifter]

@levithomason You're completely right. I think it will be easier to drop an idea Panel sub-component.

[codecov-io]

Codecov Report

Merging #1375 into master will decrease coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1375      +/-   ##
==========================================
- Coverage   99.76%   99.76%   -0.01%     
==========================================
  Files         148      149       +1     
  Lines        2556     2546      -10     
==========================================
- Hits         2550     2540      -10     
  Misses          6        6

Impacted Files	Coverage Δ
src/modules/Accordion/AccordionContent.js	100% <ø> (ø)	⬆️
src/modules/Accordion/AccordionAccordion.js	100% <100%> (ø)
src/modules/Accordion/AccordionTitle.js	100% <100%> (ø)	⬆️
src/modules/Accordion/Accordion.js	100% <100%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e1ff28a...8cb2062. Read the comment docs.

[layershifter]

@levithomason I need you there 😢

Our factories make something wrong with mergings of props. handleMenu will be never called in this example:

import React from 'react'
import { Menu } from 'semantic-ui-react'

const handleItem = (e, props) => console.log(111, props)
const handleMenu = (e, props) => console.log(222, props)

const items = [
  { key: 'editorials', active: true, name: 'Editorials', onClick: handleItem },
  { key: 'review', name: 'Reviews', onClick: handleItem },
  { key: 'events', name: 'Upcoming Events', onClick: handleItem },
]

const MenuExampleProps = () => (
  <Menu items={items} onItemClick={handleMenu} />
)

export default MenuExampleProps

I had same problem with refactored Accordion, you can see failed test.

[layershifter]

Blocked. Requires fix of #1424.

[levithomason]

Heads up, I've also opened #1445 to finish the list of callbacks in #623. This will close that issue out but will also cause a couple conflicts in this PR.

[layershifter]

@levithomason Thanks 👍 I will back there after we will solve #1424.

[layershifter]

TODO

• rebase to master
• update tests
• update typings
• check docs

[layershifter]

@levithomason I've merged with master, ready for review.

[levithomason]

Not sure about the API here, this produces invalid SUI HTML with two nested fields:

<div class="field accordion ui">
  <div class="title">
    <i class="dropdown icon"></i>Optional Details
  </div>
  <div class="field content">
    <label>Maiden Name</label>
    <div class="ui input">
      <input type="text" placeholder="Maiden Name" />
    </div>
  </div>
</div>

The Form.Field creates a field, but the content as Form.Input also creates a field. I tried to correct this by making the Accordion render as a Form.Field with a label prop, and the content render as an Input with the placeholder, however, this does not work.

I think I see where this is trying to go by allowing any component to be the Accordion and any component to be the Content, however, the CSS doesn't allow for this. We're bound to hit issues merging them.

The ui accordion should only contain children, same as the title and content. It would be ideal to have a generic Accordion component that could take any component, however, I think it would have to be JS style based and not CSS className based.

[layershifter]

I will check this example tomorrow. @levithomason do you have another remarks to this PR? After it will be merged, it will be easiest place where we can get transitions 😄

[layershifter]

Hm, @levithomason it renders same as there except CSS classes order. Can you take a look? Did I miss something?

    <div class="ui accordion field">
      <div class="title">
        <i class="icon dropdown"></i>
        Optional Details
      </div>
      <div class="content field">
        <label>Maiden Name</label>
        <input placeholder="Maiden Name" type="text">
      </div>
    </div>

[levithomason]

I'm saying I do not think the HTML is valid SUI markup. I see no supported examples of nested fields: field > field.

[layershifter]

Take a look there

<div class="ui accordion field">
  <div class="content field active"></div>
</div>

[levithomason]

Thanks for straightening me out on that! Will manually test this to refresh my memory. Likely will merge.

[levithomason]

We'll need some Before/After help on this PR description for an upgrade guide.

[layershifter]

I'll update an initial comment for instructions 👍

[layershifter]

Done

[levithomason]

Superb, thanks!

[levithomason]

Released in [email protected]

[darewreck54]

@layershifter Is there a way to work around the removal of Panel subcomponent. I found that the subcomponent really helpful in customizing the look and feel. Without this, I'm not able to do that anymore?

Example. In my case I wanted to customize the different colors of the drop down icon in the title or customize the title.

Is that possible to do it through the way the panel is defined?

[layershifter]

@darewreck54 There never was Accordion.Panel, only Accordion.Title and Accordion.Content. Both still exist.

[brianespinosa]

I wanted to customize the different colors of the drop down icon in the title or customize the title.

@darewreck54 I am assuming you want to customize the look of these elements? If so, that is not something that SUIR is doing. Your CSS will be used to customize the look of those elements. You can either write your own overrides, or use the theme building tools as part of SUI core to compile your own custom CSS theme.