New tree view integration #864

brumik · 2018-10-31T10:58:38Z

What: Added the react-wooden-tree to the storybook and initialized with a basic setup.
react-wooden-tree is a new tree view component, which should be integrated to the patternfly.

It is a modern and fast tree view component built for Red Hat as my bachelors thesis. It can be used as a navigation as well. It implements checkboxes, selects (in all form) and totally customizable trough classes.

The TreeView was built with performance in mind, with the help of benchmarks.

Additional issues:

The tree does not have design yet. Therefore this PR was created to to make the tree for other developers visible the integration to be able to help with the design.

Update from previous PR:

Continued PR #750 - Renamed the branch and was unable to reopen the PR.
Solved the issue with jest failed test with moving some dependencies in the react-wooden-tree component to the dev deps.

@Hyperkid123
@skateman
@priley86

patternfly-build · 2018-11-07T08:44:59Z

PatternFly-React preview: https://864-pr-patternfly-react-patternfly.surge.sh

coveralls · 2018-11-07T08:45:23Z

Pull Request Test Coverage Report for Build 4583

0 of 0 changed or added relevant lines in 0 files are covered.
361 unchanged lines in 32 files lost coverage.
Overall coverage decreased (-8.03%) to 70.853%

Files with Coverage Reduction	New Missed Lines	%
packages/patternfly-3/patternfly-react/src/components/Filter/FilterCategoryValueSelector.js	1	66.67%
packages/patternfly-4/react-core/src/components/Dropdown/Separator.js	1	80.0%
packages/patternfly-3/patternfly-react/src/components/LoginPage/components/LoginPageComponents/LoginFooterLinks.js	1	85.71%
packages/patternfly-3/patternfly-react/src/common/closestPolyfill.js	1	73.08%
packages/patternfly-3/patternfly-react/src/components/Filter/FilterValueSelector.js	1	66.67%
packages/patternfly-3/patternfly-react/src/components/ExpandCollapse/ExpandCollapse.js	1	96.15%
packages/patternfly-3/patternfly-react/src/components/LoginPage/components/LoginCardComponents/LoginCardInputWarning.js	1	50.0%
packages/patternfly-3/patternfly-react/src/components/DualListSelector/components/DualListBody.js	1	69.23%
packages/patternfly-3/patternfly-react/src/components/Sort/SortTypeSelector.js	2	68.75%
packages/patternfly-3/patternfly-react/src/components/Filter/FilterCategorySelector.js	2	61.11%

Totals
Change from base Build 4568:	-8.03%
Covered Lines:	3032
Relevant Lines:	3918

💛 - Coveralls

brumik · 2019-01-14T10:39:33Z

Status report

What I done:

I created the tree in PF as a separate package as @priley86 suggested in #750. Now it is working and patternfly-build was successful. Now you can check it out. The tree has nearly ready for the a11y. You can check out the progress at brumik/react-wooden-tree.

What I need to continue:

I was told to integrate the tree to the PF, so I will need some kind of design. (I am new, so I don't know in which form to expect it @Hyperkid123 ?). Getting the design should be enough to fully integrate to PF I hope. If not what else is needed to do?

@skateman

skateman · 2019-01-21T14:55:06Z

@priley86 can you please give @brumik the requested input? It would be nice to get this in so we can finally use it.

martinpovolny · 2019-01-21T15:00:35Z

I 👍 @skateman 's comment above. We need this component for ManageIQ.

Should this process take significant time (like an extra month) from now I am for moving this effort under https://github.com/manageIQ/react-ui-components.

In ManageIQ we preffer to contribute to Patternfly but if the price is months of waiting then we'd rather skip that.

priley86 · 2019-01-22T02:29:59Z

.gitignore

@@ -14,6 +14,7 @@ packages/react-icons/src/index.d.ts
 # package managers
 yarn-error.log
 lerna-debug.log
+yarn.lock



please remove yarn.lock from gitignore

you may not find it useful in some downstream apps, but in a central repository like PF, it is common to check it in.

priley86 · 2019-01-22T02:34:05Z

thanks for splitting this into a separate package @brumik.

i've requested a review from @michaelkro - he is more familiar w/ the treeview.

skateman · 2019-01-22T13:00:14Z

Related issue in MiQ: ManageIQ/manageiq-ui-classic#5178

michaelkro · 2019-01-22T18:41:52Z

packages/patternfly-3/react-tree-view/src/components/TreeView/TreeView.stories.js

+stories.addDecorator(
+  defaultTemplate({
+    title: 'Catalog Tile',
+    description: 'This is a Tree View component to display trees and hierarchical data using React Wooden Tree.'


Can we please add a link to react-wooden-tree's npmjs page

michaelkro · 2019-01-22T18:48:58Z

packages/patternfly-3/react-tree-view/src/components/TreeView/index.js

+          nodeIcon="fa fa-file-o"
+          data={this.state.tree}
+          onDataChange={this.onDataChange}
+          lazyLoad={this.lazyLoad}


Should we spread props into <Tree /> for the remaining react-wooden-tree options?

I am sorry, but I don't understand the question: what do you mean under spread?

I was thinking, say, if the consumer wants to take advantage of the loadingIcon option for <Tree />. With the current implementation, it does not seem possible to take advantage of the configuration.

If we added

<Tree ... {...this.props} />

it would allow the consuming application to make use of all the options available on this list

Or maybe, we could explicitly declare propTypes for all of the options?

So if I understood correctly, you want to get the default values of the props which are defined in the tree which is defined here and here? So it can be used in the parent component of the tree?

I cannot test but I pretty sure that you can use [String: Any] variable to define the props:

<Tree {...this.propsForTheTree} />

this should work if the propsForTheTree is defined correctly.

Definitely agree that

<Tree {...this.propsForTheTree} />

would work :) However, please forgive me if I am missing something, but that would only work if the downstream application was doing something like

import { Tree } from 'react-wooden-tree' // ... <Tree { ...this.propsForTheTree } />

I have to admit that I'm not exactly sure how our yarn workspace works exactly, but my understanding is that after this package is added, the consuming application would be doing something like

import { TreeView } from 'react-tree-view`; // ... <TreeView { ...this.propsForTheTree } />

in which case, spreading props into TreeView would not work. What do you think?

EDIT: I removed the @patternfly from the package namespace. I think we're only using that for pf4

brumik · 2019-01-24T14:51:24Z

Added the required changes. Does it need any other changes?

priley86 · 2019-01-24T15:11:09Z

packages/patternfly-3/react-tree-view/package.json

@@ -0,0 +1,37 @@
+{
+  "name": "react-tree-view",


this npm package name is already taken it seems... maybe something like patternfly-react-wooden-tree fits better?
https://www.npmjs.com/package/react-tree-view

Once you've chosen a package name, I think the last thing we need will be to add a line to the README providing instructions on how to install the module. Something like

Getting Started

npm install <package_name> --save

michaelkro · 2019-01-24T17:47:55Z

packages/patternfly-3/patternfly-react-wooden-tree/src/components/TreeView/index.js

+require('react-wooden-tree/dist/react-wooden-tree.css');
+require('./style.css');
+
+const data = [


Uh oh, sorry that I missed this before, but this hardcoded data should probably get moved out of this file. Perhaps for now, you could create a file

patternfly-react-wooden-tree/src/components/TreeView/TreeView.fixtures.js

to be used for the storybook.

Looking at the npm documentation, I think we'll want to pass the tree data down to as a prop

michaelkro · 2019-01-24T17:48:43Z

packages/patternfly-3/patternfly-react-wooden-tree/src/components/TreeView/index.js

+  constructor(props) {
+    super(props);
+
+    this.data = Tree.initTree(data);


I think we want this line to read

this.data = Tree.initTree(props.data);

Oh, we'll need a propTypes declaration too, to keep eslint from failing

packages/patternfly-3/patternfly-react-wooden-tree/src/components/TreeView/TreeView.stories.js

michaelkro · 2019-01-24T17:51:34Z

packages/patternfly-3/patternfly-react-wooden-tree/src/components/TreeView/TreeView.stories.js

+    propTables: [TreeView]
+  })(() => (
+    <div style={{ display: 'flex' }}>
+      <TreeView />


Once we've moved or imported the test data in this file, I think we want this to be

<TreeView data={data} />

michaelkro · 2019-01-24T17:59:31Z

packages/patternfly-3/patternfly-react-wooden-tree/src/components/TreeView/index.js

+  }
+
+  render() {
+    return (


Since there will be at least one known prop data, we'll want to destructure that out before spreading the remaining props into <Tree>, like the following

render() { const { data, ...remainingProps } = this.props; return ( <div className="App"> <Tree nodeIcon="fa fa-file-o" data={this.state.tree} onDataChange={this.onDataChange} lazyLoad={this.lazyLoad} {...remainingProps} /> </div> ) }

michaelkro · 2019-01-31T18:01:34Z

I think we want the reworked changes from edef1ef right? (it seems that this commit was reverted)

brumik · 2019-02-06T10:43:48Z

Yes, I messed something up, I am sorry.

brumik · 2019-02-06T11:15:06Z

Now I believe its fine. Just the tests got some error (some options are deprecated), but I think its not a problem with my package as I didn't changed tests.

skateman · 2019-02-18T06:43:46Z

@brumik please fix the yarn.lock conflict, @michaelkro if he does so, can this be merged or is there something else to do?

brumik · 2019-02-18T10:39:48Z

As @michaelkro said he is no longer working with RedHat, can you @priley86 provide what else you need after I merged the yarn.lock?

brumik · 2019-02-18T11:48:13Z

@priley86 I merged the yarn lock. The CI issues seem irrelevant.

martinpovolny · 2019-02-18T11:51:16Z

@priley86 I merged the yarn lock. The CI issues seem irrelevant.

Yes, the same issue is on master. Seems the master is broken right now.

What is the overall status of this work? Are any further changes needed? Or can this be merged now?

Thank you, @michaelkro , @brumik , @skateman, for the great work!

skateman · 2019-02-22T04:28:42Z

@priley86 could we please get this in ASAP? It's a little frustrating to have a merge conflict every second day but no progress. Thanks

karelhala · 2019-02-22T09:09:53Z

@rachael-phillips can we add some people on review process here? It's been opened for quite a few days and no activity.

dgutride · 2019-03-08T14:40:37Z

We are ready to take this in with a clean build - @brumik @martinpovolny - please contact me if you need somebody to take this on from our side so we can get it in today or Monday.

brumik · 2019-03-11T07:29:38Z

Hello, I used yarn 1.13.0 but the tests are failing: const { default: serializer, createSerializer } = require('./dist/js/build/jest/snapshot-serializer'). I don't know what is wrong.

redallen · 2019-03-11T12:46:38Z

The problem is with our Enzyme version compared to your new React version (^16.6.1). React version 16.4.0 with these Enzyme versions is confirmed working:

    "enzyme": "3.3.0",
    "enzyme-adapter-react-16.3": "1.3.0",
    "enzyme-to-json": "3.3.4",

This is a problem I've only recently discovered and I hope to make a PR for today. Our yarn.lock was actually the only thing forcing the Enzyme versions to be correct.

brumik · 2019-03-11T15:22:33Z

So what should I do? @redallen

redallen · 2019-03-11T17:32:41Z

If you want to immediately get your changes through, use the versions I just said in all package.json files and update yarn.lock. For now I'd recommend waiting on #1541 which will fix tests and then merging those changes.

jeff-phillips-18 · 2019-03-14T12:49:58Z

You should be able to rebase now and resolve the conflicts. Should then be good to go

This reverts commit 0263ba4.

This reverts commit 7b900c0.

brumik · 2019-03-18T06:36:07Z

I did it, but the test still failing with:
Cannot find module './dist/js/build/jest/snapshot-serializer' from 'snapshot-serializer.js'

jeff-phillips-18 · 2019-03-18T13:00:04Z

@redallen Can you help here?

redallen · 2019-03-18T17:01:14Z

@brumik The build looks fine on my end with tests passing. If you run yarn install to upgrade enzyme, that will likely fix it locally for you. Good to merge if we can get someone else to approve.

brumik changed the title ~~Wooden tree integration~~ New tree view integration Oct 31, 2018

brumik closed this Nov 7, 2018

brumik reopened this Nov 7, 2018

priley86 requested a review from michaelkro January 21, 2019 16:07

priley86 reviewed Jan 22, 2019

View reviewed changes

michaelkro reviewed Jan 22, 2019

View reviewed changes

priley86 reviewed Jan 24, 2019

View reviewed changes

michaelkro reviewed Jan 24, 2019

View reviewed changes

packages/patternfly-3/patternfly-react-wooden-tree/src/components/TreeView/TreeView.stories.js Show resolved Hide resolved

michaelkro reviewed Jan 24, 2019

View reviewed changes

brumik dismissed jeff-phillips-18’s stale review via 3a5c8fc March 11, 2019 07:11

Levente Berky added 16 commits March 18, 2019 08:12

Removed 'jest' from dependencies to fix testing.

d481611

Set up wooden-tree-component for storybook

47d56fc

Moved the tree view to its own package.

9e12a1c

Added props spread to the tree

5f3ebfb

Added the link to the npm page of the tree.

89927ca

Rename the package

a7e0518

Renamed the package again and added basic readme

de053da

Moved the default data to a separate file and provided propTypes

ff6aced

Reworked to re-export the original component

2230fc9

Revert "Reworked to re-export the original component"

b3e321e

This reverts commit 0263ba4.

Added dependency prop-types - passes lint

5c8e151

Revert "Revert "Reworked to re-export the original component""

946e9a7

This reverts commit 7b900c0.

Added prop types dependency to the package

43cdfa6

Updated yarn.lock file

479c01a

Removed class variable and constructor from the example

b64dc9d

Renamed to Wooden Tree View, description changes

ce54691

karelhala self-requested a review March 18, 2019 13:42

karelhala approved these changes Mar 18, 2019

View reviewed changes

jeff-phillips-18 approved these changes Mar 18, 2019

View reviewed changes

jeff-phillips-18 merged commit 9a50826 into patternfly:master Mar 18, 2019

New tree view integration #864

New tree view integration #864

Conversation

brumik commented Oct 31, 2018

patternfly-build commented Nov 7, 2018

coveralls commented Nov 7, 2018 • edited Loading

Pull Request Test Coverage Report for Build 4583

💛 - Coveralls

brumik commented Jan 14, 2019

Status report

What I done:

What I need to continue:

skateman commented Jan 21, 2019

martinpovolny commented Jan 21, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

priley86 commented Jan 22, 2019

skateman commented Jan 22, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelkro Jan 24, 2019 • edited Loading

Choose a reason for hiding this comment

brumik commented Jan 24, 2019

Choose a reason for hiding this comment

michaelkro Jan 24, 2019 • edited Loading

Choose a reason for hiding this comment

Getting Started

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelkro commented Jan 31, 2019

brumik commented Feb 6, 2019 • edited Loading

brumik commented Feb 6, 2019

skateman commented Feb 18, 2019

brumik commented Feb 18, 2019

brumik commented Feb 18, 2019

martinpovolny commented Feb 18, 2019

skateman commented Feb 22, 2019

karelhala commented Feb 22, 2019

dgutride commented Mar 8, 2019

brumik commented Mar 11, 2019

redallen commented Mar 11, 2019

brumik commented Mar 11, 2019

redallen commented Mar 11, 2019 • edited Loading

jeff-phillips-18 commented Mar 14, 2019

brumik commented Mar 18, 2019

jeff-phillips-18 commented Mar 18, 2019

redallen commented Mar 18, 2019

coveralls commented Nov 7, 2018 •

edited

Loading

martinpovolny commented Jan 21, 2019 •

edited

Loading

michaelkro Jan 24, 2019 •

edited

Loading

michaelkro Jan 24, 2019 •

edited

Loading

brumik commented Feb 6, 2019 •

edited

Loading

redallen commented Mar 11, 2019 •

edited

Loading