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

feat(Tree): Adding exclusive flag #1018

Merged
merged 20 commits into from
Mar 11, 2019
Merged

Conversation

priyankar205
Copy link
Collaborator

Adding the exclusive flag. So that only one subtree can be open at a time

@codecov
Copy link

codecov bot commented Mar 5, 2019

Codecov Report

Merging #1018 into master will decrease coverage by 0.25%.
The diff coverage is 31.25%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1018      +/-   ##
==========================================
- Coverage   81.51%   81.26%   -0.26%     
==========================================
  Files         675      673       -2     
  Lines        8755     8675      -80     
  Branches     1558     1470      -88     
==========================================
- Hits         7137     7050      -87     
- Misses       1602     1610       +8     
+ Partials       16       15       -1
Impacted Files Coverage Δ
packages/react/src/components/Tree/TreeTitle.tsx 73.68% <ø> (ø) ⬆️
packages/react/src/components/Tree/TreeItem.tsx 51.51% <14.28%> (-6.55%) ⬇️
packages/react/src/components/Tree/Tree.tsx 78.57% <44.44%> (-16.67%) ⬇️
...ams-high-contrast/components/Menu/menuVariables.ts 50% <0%> (-50%) ⬇️
...themes/teams-dark/components/Menu/menuVariables.ts 50% <0%> (-50%) ⬇️
...t/src/components/Dropdown/DropdownSelectedItem.tsx 42.85% <0%> (-26.92%) ⬇️
packages/react/src/lib/factories.ts 93.84% <0%> (-1.71%) ⬇️
...ckages/react/src/components/Button/ButtonGroup.tsx 97.22% <0%> (-0.08%) ⬇️
...kages/react/src/themes/teams-dark/siteVariables.ts 100% <0%> (ø) ⬆️
...es/react/src/components/Reaction/ReactionGroup.tsx 100% <0%> (ø) ⬆️
... and 22 more

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 7760f24...728e753. Read the comment docs.

@codecov
Copy link

codecov bot commented Mar 5, 2019

Codecov Report

Merging #1018 into master will decrease coverage by 0.06%.
The diff coverage is 54.54%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1018      +/-   ##
==========================================
- Coverage   81.74%   81.67%   -0.07%     
==========================================
  Files         687      687              
  Lines        8813     8835      +22     
  Branches     1493     1500       +7     
==========================================
+ Hits         7204     7216      +12     
- Misses       1594     1604      +10     
  Partials       15       15
Impacted Files Coverage Δ
packages/react/src/components/Tree/TreeTitle.tsx 73.68% <ø> (ø) ⬆️
packages/react/src/components/Tree/TreeItem.tsx 58.06% <28.57%> (ø) ⬆️
packages/react/src/components/Tree/Tree.tsx 74.41% <61.53%> (-20.82%) ⬇️

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 4e4697e...4a20808. Read the comment docs.

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

handleTreeItemOverrides = predefinedProps => ({
onOpenChanged: (e, treeItemProps) => {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
onOpenChanged: (e, treeItemProps) => {
onOpenChanged: (e: React.SyntheticEvent, treeItemProps: TreeItemProps) => {

@layershifter
Copy link
Member

layershifter commented Mar 6, 2019

This PR contains the chicken-egg problem as I mentioned originally in #479: we have control of activeIndex/clickedIndex in Tree and open in TreeItem 🤔

I expect from API something like this:

const items = [
  {
    key: '1',
    activeIndex: 0, // < ---
    title: 'one',
    items: [
      {
        key: '2',
        activeIndex: 0, // < ---
        title: 'one one',
        items: [
          {
            key: '3',
            title: 'one one one',
          },
        ],
      },
    ],
  },
  {
    key: '4',
    title: 'two',
    items: [
      {
        key: '5',
        title: 'two one',
      },
    ],
  },
]

<Tree items={items} activeIndex={1} />

I suggest to move all activeIndex functionality to TreeItem and use factory in Tree:

render() {
  return TreeItem.create({ as, items })
}

@mnajdova what do you think?

@mnajdova
Copy link
Contributor

mnajdova commented Mar 6, 2019

This PR contains the chicken-egg problem as I mentioned originally in #479: we have control of activeIndex/clickedIndex in Tree and open in TreeItem 🤔

I agree with your point. I would restrain for suggesting what the changes should be (although I agree with you). The goal of it should be, it should be able to auto control it's state and allow the user to control it. With this changes I don't think we are following this pattern. @priyankar205 please consider the comments from @layershifter - let's make this component follow the stardust principles implemented in all other components.

@priyankar205
Copy link
Collaborator Author

@mnajdova @layershifter No other component supports nested subcomponents. I will try to make the suggested changes.

@priyankar205
Copy link
Collaborator Author

This PR contains the chicken-egg problem as I mentioned originally in #479: we have control of activeIndex/clickedIndex in Tree and open in TreeItem 🤔

I expect from API something like this:

const items = [
  {
    key: '1',
    activeIndex: 0, // < ---
    title: 'one',
    items: [
      {
        key: '2',
        activeIndex: 0, // < ---
        title: 'one one',
        items: [
          {
            key: '3',
            title: 'one one one',
          },
        ],
      },
    ],
  },
  {
    key: '4',
    title: 'two',
    items: [
      {
        key: '5',
        title: 'two one',
      },
    ],
  },
]

<Tree items={items} activeIndex={1} />

I suggest to move all activeIndex functionality to TreeItem and use factory in Tree:

render() {
  return TreeItem.create({ as, items })
}

@mnajdova what do you think?

If I move activeIndex to the TreeItem then how do I control the sibling TreeItems. How do I close the rest when the exclusive flag is there

@priyankar205 priyankar205 merged commit d2c5cee into master Mar 11, 2019
@delete-merged-branch delete-merged-branch bot deleted the priyankar/tree-exclusive-flag branch March 11, 2019 10:01
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants