fix(Accordion) fix wrong indexes when using Accordion.Title and Accordion.Content

[saitonakamura]

RFC

fixes #773

I fix wrong indexes but I find the code kinda ugly. It would be better if we don't need to copy index into local variable, but without it its buggy, i'm not sure why.

Another way: still use child.index but divide it by two like this

return Children.map(children, (child, i) => {
  const currentIndex = i / 2 
  // 0 / 2 = 0
  // 1 / 2 = 1
  // 2 / 2 = 1
  // 3 / 2 = 1
  // etc.
  ...
})

But its hard to read

[codecov-io]

Current coverage is 100% (diff: 100%)

No coverage report found for master at 608ae24.

Powered by Codecov. Last update 608ae24...af26688

[levithomason]

Could we remove currentIndex from this block and the isContent block and just use titleIndex and contentIndex respectively? I think it is a little more clear.

[saitonakamura]

I did that at first

if (isTitle) {
  const isActive = _.has(child, 'props.active') ? child.props.active : activeIndex === titleIndex
  const onClick = (e) => {
    this.handleTitleClick(e, titleIndex)
    if (child.props.onClick) child.props.onClick(e, titleIndex)
  }
  titleIndex++
  return cloneElement(child, { ...child.props, active: isActive, onClick })
}

if (isContent) {
  const isActive = _.has(child, 'props.active') ? child.props.active : activeIndex === contentIndex
  contentIndex++
  return cloneElement(child, { ...child.props, active: isActive })
}

But if you do this, than all titles will have index 3 in the callback. Like the lambda gets not only lazy evaluation, but lazy creation, or value is passed by reference.

[levithomason]

Ah, indeed. It is a reference and there is a single titleIndex available. Good catch.

[levithomason]

Thanks for this test. Could we also add one more test similar to this that asserts that the correct title/content are "active" given an activeIndex of 0, 1, and 2? Something like this added to describe('activeIndex'), note I did not test this so it could have issues:

it('sets the correct pair of title/content active', () => {
  const wrapper = shallow(
    <Accordion>
      <Accordion.Title />
      <Accordion.Content />
      <Accordion.Title />
      <Accordion.Content />
      <Accordion.Title />
      <Accordion.Content />
    </Accordion>
  )
  wrapper.setProps({ activeIndex: 0 })
  wrapper.childAt(0).should.have.prop('active', true)
  wrapper.childAt(1).should.have.prop('active', true)

  wrapper.setProps({ activeIndex: 1 })
  wrapper.childAt(2).should.have.prop('active', true)
  wrapper.childAt(3).should.have.prop('active', true)

  wrapper.setProps({ activeIndex: 2 })
  wrapper.childAt(4).should.have.prop('active', true)
  wrapper.childAt(5).should.have.prop('active', true)
})

[saitonakamura]

Of course, sure I can add this

[levithomason]

Released in semantic-ui-react@0.60.7.