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

Commit

Permalink
Changhes for adding exclusive flag
Browse files Browse the repository at this point in the history
  • Loading branch information
PRIYANKAR GUPTA committed Mar 4, 2019
1 parent 4a0cdab commit fcd8a68
Show file tree
Hide file tree
Showing 5 changed files with 93 additions and 11 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
import * as React from 'react'
import { Icon, Tree } from '@stardust-ui/react'

const items = [
{
key: '1',
title: 'one',
items: [
{
key: '2',
title: 'one one',
items: [
{
key: '3',
title: 'one one one',
},
],
},
{
key: '6',
title: 'one two',
items: [
{
key: '7',
title: 'one two one',
},
],
},
],
},
{
key: '4',
title: 'two',
items: [
{
key: '5',
title: 'two one',
},
],
},
]

const titleRenderer = (Component, { content, open, hasSubtree, ...restProps }) => (
<Component open={open} hasSubtree={hasSubtree} {...restProps}>
{hasSubtree && <Icon name={open ? 'arrow down' : 'arrow right'} />}
<span>{content}</span>
</Component>
)

const TreeExclusiveExample = () => (
<Tree items={items} renderItemTitle={titleRenderer} exclusive={true} />
)

export default TreeExclusiveExample
5 changes: 5 additions & 0 deletions docs/src/examples/components/Tree/Types/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,11 @@ const Types = () => (
description="A Tree with customized title rendering."
examplePath="components/Tree/Types/TreeTitleCustomizationExample"
/>
<ComponentExample
title="Exclusive Flag"
description="A Tree with only one subtree open at a time."
examplePath="components/Tree/Types/TreeExclusiveExample"
/>
</ExampleSection>
)

Expand Down
19 changes: 17 additions & 2 deletions packages/react/src/components/Tree/Tree.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -50,23 +50,38 @@ class Tree extends UIComponent<ReactProps<TreeProps>> {
...commonPropTypes.createCommon({
content: false,
}),
exclusive: PropTypes.bool,

This comment has been minimized.

Copy link
@mnajdova

mnajdova Mar 5, 2019

Contributor

Should be added in the prop's interface too.

items: customPropTypes.collectionShorthand,
renderItemTitle: PropTypes.func,
rtlAttributes: PropTypes.func,
}

state = {
selectedIndex: -1,
}

public static defaultProps = {
as: 'ul',
accessibility: defaultBehavior,
}

clickHandler = (e, index) => {
this.setState({
selectedIndex: index,
})
}

renderContent() {
const { items, renderItemTitle } = this.props
const { items, renderItemTitle, exclusive } = this.props

return _.map(items, item =>
return _.map(items, (item, index) =>
TreeItem.create(item, {
defaultProps: {
index,
exclusive,
renderItemTitle,
open: index === this.state.selectedIndex,
onClick: this.clickHandler,

This comment has been minimized.

Copy link
@mnajdova

mnajdova Mar 5, 2019

Contributor

The click handler should be added in the override props, and after the action you are providing, you should call the user's onClick if it is defined. See how it is done in the TreeItem component.

},
}),
)
Expand Down
22 changes: 15 additions & 7 deletions packages/react/src/components/Tree/TreeItem.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import TreeTitle from './TreeTitle'
import { defaultBehavior } from '../../lib/accessibility'
import { Accessibility } from '../../lib/accessibility/types'
import {
AutoControlledComponent,
UIComponent,
childrenExist,
customPropTypes,
createShorthandFactory,
Expand Down Expand Up @@ -52,24 +52,25 @@ export interface TreeItemState {
open?: boolean
}

class TreeItem extends AutoControlledComponent<ReactProps<TreeItemProps>, TreeItemState> {
class TreeItem extends UIComponent<ReactProps<TreeItemProps>, TreeItemState> {
static create: Function

static className = 'ui-tree__item'

static displayName = 'TreeItem'

static autoControlledProps = ['open']

static propTypes = {
...commonPropTypes.createCommon({
content: false,
}),
defaultOpen: PropTypes.bool,
items: customPropTypes.collectionShorthand,
index: PropTypes.number,
exclusive: PropTypes.bool,

This comment has been minimized.

Copy link
@mnajdova

mnajdova Mar 5, 2019

Contributor

Should be added in the prop's interface too.

open: PropTypes.bool,
renderItemTitle: PropTypes.func,
treeItemRtlAttributes: PropTypes.func,
onClick: PropTypes.func,
title: customPropTypes.itemShorthand,
}

Expand All @@ -78,19 +79,24 @@ class TreeItem extends AutoControlledComponent<ReactProps<TreeItemProps>, TreeIt
accessibility: defaultBehavior,
}

public state = {

This comment has been minimized.

Copy link
@mnajdova

mnajdova Mar 5, 2019

Contributor

You can't initialize the state here, as using the prop when it is in controlled mode won't work. It will by default be false.

open: false,
}

handleTitleOverrides = predefinedProps => ({
onClick: (e, titleProps) => {
e.preventDefault()
this.trySetState({
this.setState({

This comment has been minimized.

Copy link
@mnajdova

mnajdova Mar 5, 2019

Contributor

What was the reason for this to be changed? We should use trySetState when we have AutoControlled component

This comment has been minimized.

Copy link
@priyankar205

priyankar205 Mar 5, 2019

Collaborator

It is no more an auto controlled component because when we have the exclusive flag the parent controls the state.

open: !this.state.open,
})
_.invoke(predefinedProps, 'onClick', e, titleProps)
_.invoke(this.props, 'onClick', e, this.props.index, titleProps)

This comment has been minimized.

Copy link
@mnajdova

mnajdova Mar 5, 2019

Contributor

You don't need the second onClick, the first one is enough, the titleProps will already containg the prop.index as a second parametar of the onClick handler. The signature of the handlers it he following:

onClick(e, props) => {
// access the index as props.index
...
}

This comment has been minimized.

Copy link
@priyankar205

priyankar205 Mar 5, 2019

Collaborator

treeItem props have titleProps titleProps don't

This comment has been minimized.

Copy link
@mnajdova

mnajdova Mar 5, 2019

Contributor

I don't think I understand this..

This comment has been minimized.

Copy link
@priyankar205

priyankar205 Mar 5, 2019

Collaborator

We need both here as in accordian.

},
})

renderContent() {
const { items, title, renderItemTitle } = this.props
const { open } = this.state
const open = this.props.exclusive ? this.props.open : this.state.open

This comment has been minimized.

Copy link
@mnajdova

mnajdova Mar 5, 2019

Contributor

The only source of truth for the open value should always be the state (you are using AutoControlled component, which means that you don't have to care what is the value of the prop). The exclusive prop should only be propagated to the Tree generated, as it means nothing in the context of one TreeItem.

This comment has been minimized.

Copy link
@priyankar205

priyankar205 Mar 5, 2019

Collaborator

When there is no exclusive flag the TreeItem controls the open state. But when there is an exclusive flag the Tree component controls the open state (based on the index). The exclusive flag also needs to be passed down to the subtree.

This comment has been minimized.

Copy link
@mnajdova

mnajdova Mar 5, 2019

Contributor

Usually, how this should work is, the Tree is always controlling the open state, but the TreeItem itself can have some handler that will be invoked when the open prop should be change. See how the MenuItem, have the method onActiveChanged - should be something similar.


const hasSubtree = !!(items && items.length)

Expand All @@ -104,7 +110,9 @@ class TreeItem extends AutoControlledComponent<ReactProps<TreeItemProps>, TreeIt
render: renderItemTitle,
overrideProps: this.handleTitleOverrides,
})}
{hasSubtree && open && <Tree items={items} renderItemTitle={renderItemTitle} />}
{hasSubtree && open && (
<Tree items={items} renderItemTitle={renderItemTitle} exclusive={this.props.exclusive} />
)}
</>
)
}
Expand Down
4 changes: 2 additions & 2 deletions packages/react/src/components/Tree/TreeTitle.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -61,11 +61,11 @@ class TreeTitle extends UIComponent<ReactProps<TreeTitleProps>> {

return (
<ElementType
className={classes.root}
onClick={this.handleClick}
{...accessibility.attributes.root}
{...rtlTextContainer.getAttributes({ forElements: [children, content] })}
{...unhandledProps}
className={classes.root}

This comment has been minimized.

Copy link
@mnajdova

mnajdova Mar 5, 2019

Contributor

This change is bad. With this the user will never be able to override the className and onClick handler. See how the mechanism of the onClick override is done in the other components.

This comment has been minimized.

Copy link
@priyankar205

priyankar205 Mar 5, 2019

Collaborator

I am fixing a bug here :) . The className and onClick were getting overridden. Now it is in parity with other components.

This comment has been minimized.

Copy link
@mnajdova

mnajdova Mar 5, 2019

Contributor

I understand for the onClick prop as I see that we have handle method for it, but for the className, the classes.root are already merged with the users className, so it can safely be first. The className is handled prop anyways.

This comment has been minimized.

Copy link
@layershifter

layershifter Mar 5, 2019

Member

The className and onClick were getting overridden.

  • className will be merged in renderComponent()
  • onClick is overridden because it was not defined in propTypes
onClick={this.handleClick}
>
{childrenExist(children) ? children : content}
</ElementType>
Expand Down

0 comments on commit fcd8a68

Please sign in to comment.