Update: Tree picker node component to support child nodes #443

pphminions · 2016-11-21T06:00:25Z

Support tree picker node component with child nodes. ~~If isChildNode property of a node is true, then it will contain a padding to indent correctly .~~ If node is child it will have child-node class, allowing consumer to style it accordingly.

mdotwills · 2016-11-21T06:03:57Z

I didn't notice the isChildNode property on the node type props, maybe update that too and set the default to false with default props?

omgaz · 2016-11-21T06:09:01Z

src/components/adslotUi/TreePickerNodeComponent.jsx

+      <GridRow dts={`${_.kebabCase(itemType)}-${node.id}`} >
+        {node.isChildNode ?
+          <GridCell>
+            <div className="child-node" />


If this isn't actually a child-node can we call this child-node-indent-spacer maybe? But again, would much rather see this purely a css/class related thing on the top node.

omgaz · 2016-11-21T06:13:00Z

src/components/adslotUi/TreePickerNodeComponent.jsx

@@ -58,7 +58,13 @@ const TreePickerNodeComponent = ({

  return (
    <div className={`${baseClass}`}>


Why not add the class, child-node here, and apply the padding to .child-node > .grid-row-component? It'd be a lot cleaner than adding extra conditionals in the markup and adding extra dom nodes and cells to create something that is purely a spacer.

This is the first thing I tried @omgaz , if you add a padding here it looks like below. The padding apply to the bottom boarder as well
May be I am missing something

omgaz · 2016-11-21T06:13:48Z

test/components/adslotUi/TreePickerNodeComponentTest.jsx

@@ -127,7 +127,7 @@ describe('TreePickerNodeComponent', () => {
    />);

    const rowElement = component.find({ dts: `${_.kebabCase(itemType)}-${cbrNode.id}` });
-    const valueCellElement = rowElement.prop('children')[3];
+    const valueCellElement = rowElement.prop('children')[4];


If you go the css route none of this would have to change either.

pphminions · 2016-11-21T22:14:37Z

@mdotwills - added isChildNode as a prop of the node, but there I don't see any of the other similar properties having the default value, for example prop isExpandable

pphminions · 2016-11-21T23:45:15Z

@omgaz, @mdotwills - incorporated the review comments

OndrejCholeva · 2016-11-21T23:57:05Z

src/helpers/propTypes/TreePickerPropTypes.js

@@ -14,6 +14,7 @@ export default {
    ).isRequired,
    type: PropTypes.string.isRequired,
    value: PropTypes.number,
+    isChildNode: PropTypes.bool,


LGTM, just move it before isExpandable? 🔠

OndrejCholeva · 2016-11-22T00:40:36Z

src/helpers/propTypes/TreePickerPropTypes.js

@@ -4,6 +4,7 @@ import { SvgSymbol } from 'alexandria-adslot';
 export default {
  node: PropTypes.shape({
    id: PropTypes.string.isRequired,
+    isChildNode: PropTypes.bool,


Isn't the isChildNode computable from (or at least related to) the path prop?
Also elsewhere we are checking _.isEmpty(node.ancestors)) to figure out if it is child node or not.

@omgaz thoughts?

my understanding is 'path' and 'node.ancestors' is used to display the ancestor name with the node label.
I want something purely to add just a padding

example for the use of 'path' and 'ancestors'

@pphminions my point is that we should either think about it from higher level perspective and refactor existing code to use the isChildNode prop (to avoid duplication and confusion) or name it extraPadding if it doesn't really have much to do with path and is only a visual representation.

I know the ancestors and path is mostly coming from geo segments and the way tree picker used to be only used for targeting, but I think we should add props wisely with some plan/vision. That's why I am calling Gary, as a I believe he knows the plan.

Just note that this change it not for the targeting picker. and this change should not effect that as well. This change is required for the sites and publisher modal

Ah, I know for now it is only required for a site and publishers, but that is quite short sight solution to just go with minimal change required at the moment (why not call it isSite than?). Those components still should have quite similar props and usage.

At the end of the day those two are representation of the same thing:

Shouldn't we isolate the adslot-ui from direct-web? my feeling is isSite more direct-web related term :)

(the isSite was a joke to show that adding props because of specific need at time may lead to bad code design)

IIRC

Ancestors with type make up that (State in AU) as (type in ancestor)

Path makes up breadcrumbs

We could still pursue with adding an is-child class but, as Ondrej mentioned, calculated from whether we have ancestors/path rather than defined by the user. We don't need to add or pass an extra prop. We just apply it to all child nodes; but what about children's childrens? Maybe we need an is-child and child-depth-x? So for the future we know how deep.

Then direct-web can choose to style this how it pleases. This way we provide the information about the node, without dictating how we should style it.

If that's the case, styling in adslot-ui isn't so maintainable or extensible, we'd either end up with all pickers showing this indentation (which we don't want) or we have to add a prop type flag to decide whether or not we style it one way or the other (however adding proptypes to markup that dictate css/style in this case creates too much coupling.

@omgaz, @OndrejCholeva - removed the css style for child-node and will handle that from the direct-web

OndrejCholeva · 2016-11-23T03:35:49Z

src/components/adslotUi/TreePickerNodeComponent.jsx

@@ -57,7 +57,7 @@ const TreePickerNodeComponent = ({
  const labelCellProps = expanderElement ? { onClick: expandNodeBound } : {};

  return (
-    <div className={`${baseClass}`}>
+    <div className={node.isChildNode ? `${baseClass} child-node` : `${baseClass}`}>


if I read @omgaz comment below correctly, this should become:

<div className={node.path || node.ancestors ? `${baseClass} child-node` : `${baseClass}`}>

without need to have the isChildNode prop.

If I put path or ancestor, it will effect the ui in unexpected ways. For example, if I set path in publisher modal, it will add something which is not there in the expected design
(please ignore the incomplete path, this is just for example)

I tried this with the current adslot-ui version of the direct-web, might be slightly different in the new version

That is why I need something which is not effecting other properties in any other way. @omgaz is having another node property as isChildNode is okay?

Please refer the Brief sites card for the expected design

I think that Gary meant also to hide the path text in direct-web (if unwanted).

Then direct-web can choose to style this how it pleases. This way we provide the information about the node, without dictating how we should style it.

so the css will look like (example):

.child-node { padding-left: 30px; .treepickernode-component-metadata { display: none; }

omgaz · 2016-11-29T03:23:58Z

src/components/Main.jsx

-        { id: '2', label: 'Norfolk Island', path: [{ id: '11', label: 'AU' }], type: '', isSelectable: false },
-        { id: '3', label: 'Queensland', path: [{ id: '12', label: 'AU' }], type: '' },
+        { id: '2', label: 'Norfolk Island', path: [], type: '', isSelectable: false },
+        { id: '3', label: 'Queensland', path: [{ id: '12', label: 'AU' }], type: '', isChildNode: true },


We can remove this now.

pphminions · 2016-11-29T05:18:25Z

@omgaz - Incorporated the review comments

omgaz · 2016-12-07T00:11:30Z

Merging 🚀

pphminions force-pushed the tree-picker-node-child-node branch from a854f13 to 0ee55bf Compare November 21, 2016 06:09

omgaz reviewed Nov 21, 2016

View reviewed changes

pphminions force-pushed the tree-picker-node-child-node branch from 0ee55bf to 049b353 Compare November 21, 2016 22:10

pphminions force-pushed the tree-picker-node-child-node branch 2 times, most recently from 7fa3a61 to 51847e7 Compare November 21, 2016 23:45

OndrejCholeva reviewed Nov 21, 2016

View reviewed changes

pphminions force-pushed the tree-picker-node-child-node branch from 51847e7 to e75c7aa Compare November 22, 2016 00:14

OndrejCholeva reviewed Nov 22, 2016

View reviewed changes

pphminions force-pushed the tree-picker-node-child-node branch from e75c7aa to 8e453d9 Compare November 23, 2016 03:26

OndrejCholeva reviewed Nov 23, 2016

View reviewed changes

pphminions force-pushed the tree-picker-node-child-node branch from 8e453d9 to a8c4b01 Compare November 23, 2016 23:11

omgaz reviewed Nov 29, 2016

View reviewed changes

pphminions force-pushed the tree-picker-node-child-node branch from a8c4b01 to 8e1746d Compare November 29, 2016 05:15

omgaz force-pushed the tree-picker-node-child-node branch 2 times, most recently from 343a70f to 6054fbf Compare December 6, 2016 23:56

Update: Tree picker node component to support child nodes

8e1c153

omgaz force-pushed the tree-picker-node-child-node branch from 6054fbf to 8e1c153 Compare December 7, 2016 00:03

omgaz merged commit fc5bb7e into master Dec 7, 2016

omgaz deleted the tree-picker-node-child-node branch December 7, 2016 00:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update: Tree picker node component to support child nodes #443

Update: Tree picker node component to support child nodes #443

pphminions commented Nov 21, 2016 •

edited by OndrejCholeva

Loading

mdotwills commented Nov 21, 2016

omgaz Nov 21, 2016

omgaz Nov 21, 2016

pphminions Nov 21, 2016 •

edited

Loading

omgaz Nov 21, 2016

pphminions commented Nov 21, 2016

pphminions commented Nov 21, 2016

OndrejCholeva Nov 21, 2016

OndrejCholeva Nov 22, 2016

pphminions Nov 22, 2016 •

edited

Loading

OndrejCholeva Nov 22, 2016 •

edited

Loading

pphminions Nov 22, 2016 •

edited

Loading

OndrejCholeva Nov 22, 2016 •

edited

Loading

pphminions Nov 22, 2016 •

edited

Loading

OndrejCholeva Nov 22, 2016

omgaz Nov 22, 2016

pphminions Nov 23, 2016

OndrejCholeva Nov 23, 2016 •

edited

Loading

pphminions Nov 23, 2016 •

edited

Loading

pphminions Nov 23, 2016 •

edited

Loading

OndrejCholeva Nov 23, 2016 •

edited

Loading

omgaz Nov 29, 2016

pphminions commented Nov 29, 2016

omgaz commented Dec 7, 2016

		@@ -58,7 +58,13 @@ const TreePickerNodeComponent = ({

		return (
		<div className={`${baseClass}`}>

Update: Tree picker node component to support child nodes #443

Update: Tree picker node component to support child nodes #443

Conversation

pphminions commented Nov 21, 2016 • edited by OndrejCholeva Loading

mdotwills commented Nov 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pphminions Nov 21, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pphminions commented Nov 21, 2016

pphminions commented Nov 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pphminions Nov 22, 2016 • edited Loading

Choose a reason for hiding this comment

OndrejCholeva Nov 22, 2016 • edited Loading

Choose a reason for hiding this comment

pphminions Nov 22, 2016 • edited Loading

Choose a reason for hiding this comment

OndrejCholeva Nov 22, 2016 • edited Loading

Choose a reason for hiding this comment

pphminions Nov 22, 2016 • edited Loading

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

OndrejCholeva Nov 23, 2016 • edited Loading

Choose a reason for hiding this comment

pphminions Nov 23, 2016 • edited Loading

Choose a reason for hiding this comment

pphminions Nov 23, 2016 • edited Loading

Choose a reason for hiding this comment

OndrejCholeva Nov 23, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pphminions commented Nov 29, 2016

omgaz commented Dec 7, 2016

pphminions commented Nov 21, 2016 •

edited by OndrejCholeva

Loading

pphminions Nov 21, 2016 •

edited

Loading

pphminions Nov 22, 2016 •

edited

Loading

OndrejCholeva Nov 22, 2016 •

edited

Loading

pphminions Nov 22, 2016 •

edited

Loading

OndrejCholeva Nov 22, 2016 •

edited

Loading

pphminions Nov 22, 2016 •

edited

Loading

OndrejCholeva Nov 23, 2016 •

edited

Loading

pphminions Nov 23, 2016 •

edited

Loading

pphminions Nov 23, 2016 •

edited

Loading

OndrejCholeva Nov 23, 2016 •

edited

Loading