New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ExpandCollapse] `open` prop behaviour #892

Open
theetrain opened this Issue Jan 15, 2019 · 4 comments

Comments

Projects
None yet
4 participants
@theetrain
Copy link
Member

theetrain commented Jan 15, 2019

Why

The ExpandCollapse/Accordion React component is widely used across TELUS applications. In an effort to improve documentation and component behaviour, the TDS core team would like to gather insights on the usage of its open prop.

What

In a recent release, @tds/core-expand-collapse@1.2.4 (see #879), we patched ExpandCollapse to not have collapsing behaviour when calling this.setState() from a component that is a child of <ExpandCollapse.Panel />. After calling this.setState() it is then no longer possible to change which panels are open within that ExpandCollapse using the open prop.

The open prop is documented as follows:

A list of panel identifiers to programmatically open.

From this description, no assumptions can be made about whether the open prop:

  • A: should only be used for initial opened state of Panels, or
  • B: is always able to control the opened state of its Panels.

The question

As consumers of ExpandCollapse, which of the above behaviours do you expect from the open prop?

Animated examples

Code snippet used in examples below
class MyApp extends React.Component {
  state = {
    checked: false,
    open: []
  }

  changeHandler = () => {
    this.setState((previous) => {
      return { checked: !previous.checked }
    })
  }

  closePanel = () => {
    this.setState({ open: [] })
  }

  openPanel = () => {
    this.setState({ open: ['features'] })
  }

  render() {
    return (
      <React.Fragment>
        <ExpandCollapse tag="h2" open={this.state.open}>
          <ExpandCollapse.Panel id="features" header="Features">
            <Checkbox
              label="Call setState() within onChange handler"
              name="hello"
              value="Hii"
              checked={this.state.checked}
              onChange={this.changeHandler}
            />
            <Button onClick={this.closePanel}>Close panel (via open prop)</Button>
          </ExpandCollapse.Panel>
        </ExpandCollapse>
        <Button onClick={this.openPanel}>Open panel (via open prop)</Button>
      </React.Fragment>
    )
  }
}

ExpandCollapse v1.2.3
Clicking the Panel heading and then calling setState from child component closes the Panel, due to it re-rendering with an empty initial state. The open prop can always control Panel state.

screen recording 2019-01-15 at 02 55 pm

ExpandCollapse v1.2.4
Clicking the Panel heading and then closing via props does not work. The open prop can only be used to control Panel state when the user does not change the state by clicking a Panel heading.

screen recording 2019-01-15 at 02 42 pm

How

Depending on consumer expectations of ExpandCollapse, we see two potential outcomes:

A. Keep ExpandCollapse as a class component, use open only for initial state

By modifying existing documentation, we can help consumers make the right assumptions over the behaviour of the open prop being an initial state setter, and keep ExpandCollapse v.1.2.4 as-is.

B. Change ExpandCollapse into a functional component, provide an optional state controller

As a future development, we can produce a stateless functional component, which involves the following steps:

  1. Revert the 'fix' made to ExpandCollapse 1.2.4, and deploy 1.2.5, which restores functionality as seen in 1.2.3
  2. Add documentation to ExpandCollapse to recommend controlling the state of ExpandCollapse manually via the open prop. That means when you use Checkbox (or anything inside ExpandCollapse.Panel that calls this.setState), your application should then be sure to update the open prop. We will also document recommendations.
  3. Down the line, we'll release ExpandCollapse 2.0.0 to be a functional component with state being handled by the application, or our forthcoming WithState component wrapper that can be used for Sitebuilder/simple applications.

Who

Meta

  • TDS component: @tds/core-expand-collapse@1.2.4
  • Willing to develop solution: Yes
  • Has workaround: No
  • High impact: ?
@DJDA

This comment has been minimized.

Copy link
Contributor

DJDA commented Jan 21, 2019

☑Option B please !

@marcod1419

This comment has been minimized.

Copy link
Contributor

marcod1419 commented Jan 23, 2019

We have decided to move forward with Option B! @DJDA

@theetrain

This comment has been minimized.

Copy link
Member Author

theetrain commented Jan 25, 2019

Before moving forward, we will gather a second opinion from @invalidred.

@invalidred

This comment has been minimized.

Copy link

invalidred commented Jan 25, 2019

Option B would be cleaner, however there is nothing wrong with Option A with slight modification where open is a controlled prop and the introduction of an initialOpen state initializer prop.

The idea is if the consuming code passes open then it controls the state. For existing clients who don't want to control state, they can rename open to initialOpen and they are good to go.

A good example of controlled props can be found here notice the getState() func in line 11

This will be a breaking change, however the API does not change significantly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment