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

feat(Tree): add Component #479

Merged
merged 79 commits into from
Nov 28, 2018
Merged

feat(Tree): add Component #479

merged 79 commits into from
Nov 28, 2018

Conversation

priyankar205
Copy link
Collaborator

tree

  • Implemented the Tree component including the Teams theme
  • Work Left: Support for other themes. Implement accessibility props.

@levithomason
Copy link
Member

@layershifter could you please take a look at this component structure and API?

Copy link
Contributor

@kuzhelov kuzhelov left a comment

Choose a reason for hiding this comment

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

have couple of questions that I would like to clarify:

  • could you, please, suggest how it would be possible to pass some arbitrary content as tree's child (other than just plain string), like in the following case:
items: [
  <span dir='auto'>Hello from component!</span>,
  <span>for more info, click <button>here</button>
]

The only way how it could be achieved now seems to be the following:

items: [
  {
    title: <span dir='auto'>Hello from component!</span>
  },
  {
     title: <span>for more info, click <button>here</button>
  }

This syntax is quite verbose for achieving this simple task. Note that this problem might be alleviated by passing title property as a default one for TreeItem component:

TreeItem.create = createShorthandFactory(TreeItem, 'title')

Thank you!

@priyankar205
Copy link
Collaborator Author

@kuzhelov : I agree to your first point. I will update the code to TreeItem.create = createShorthandFactory(TreeItem, 'title')
For the second question: As the tree is represented by an 'ul' element which cannot have any child except 'li' no property can be mapped. I will update the code for that.

CHANGELOG.md Outdated Show resolved Hide resolved
...childrenComponentPropTypes,
accessibility: PropTypes.func,
items: customPropTypes.collectionShorthand,
nested: PropTypes.bool,
Copy link
Contributor

Choose a reason for hiding this comment

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

this prop seems to be not used anywhere - am I missing something (please, just reference the line in code in that case)? Thank you

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I had added a class. Got deleted in refactoring perhaps. Also it might be needed for accessibility props.

Copy link
Contributor

Choose a reason for hiding this comment

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

so, if it is not used anywhere now, lets delete it - even if it will be used for accessibility later, we will add it. Lets limit the API to only those things that are necessary

@kuzhelov
Copy link
Contributor

@priyankar205
thanks a lot for hour response. One moment that I still would like to clarify, though - the one that is related to the question of using content other than plain string for tree leafs.

While I totally agree that <ul> should contain only <li> elements, at the same time it is still valid that we could, at least in theory, provide some content that is more complex than plain string to this <li> element, right? Like in the following example:

<ul>
  <li> <span>I am not just a string</span> <li>
  <li> Me neither, click here if you disagree: <button>Disagree!</button> </li>
</ul>

So, to me it seems that current implementation is unable to handle these cases - in a sense, as I've already mentioned before, client will need to provide this content as a value for title prop, that is completely non-obvious (why header if semantics of this is a content). Please, just verify or object my assumptions, so that we would be able to have mutual understanding of where we are and what are the next steps that should be planned.

Thank you!

@priyankar205
Copy link
Collaborator Author

priyankar205 commented Nov 28, 2018

@kuzhelov We can handle these cases with the renderItemTitle function for now as in the second example. Also I see that other components like List also fail to handle these cases directly.

@priyankar205 priyankar205 merged commit 837c7d1 into master Nov 28, 2018
@layershifter layershifter deleted the priyankar/tree-component branch January 10, 2019 11:25
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants