-
Notifications
You must be signed in to change notification settings - Fork 418
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Tree Grid Component #2181
Add Tree Grid Component #2181
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aswinshenoy please see if there's a way to solve the rendering issues. I think a containing wrapper with a set height might help fix the extending lines, but I'm unsure. Let us know if you need help!
…react into tree-grid # Conflicts: # components/storybook-stories.js
import TreeGrid from '~/components/tree-grid'; | ||
import IconSettings from '~/components/icon-settings'; | ||
|
||
const data = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there were no nested rows, then a dev would use a DataTable. I don't think we need this example and base should be nested.
import TreeGrid from '~/components/tree-grid'; | ||
import IconSettings from '~/components/icon-settings'; | ||
|
||
const data = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The columns should be child components. See DataTable
component.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is the Lightning component data for instance.
({ // eslint-disable-line
init: function (cmp) {
var columns = [
{
type: 'text',
fieldName: 'accountName',
label: 'Account Name',
initialWidth: 300
},
{
type: 'number',
fieldName: 'employees',
label: 'Employees'
},
{
type: 'phone',
fieldName: 'phone',
label: 'Phone Number'
},
{
type: 'url',
fieldName: 'accountOwner',
label: 'Account Owner',
typeAttributes: {
label: { fieldName: 'accountOwnerName' }
}
},
{
type: 'text',
fieldName: 'billingCity',
label: 'Billing City'
}
];
cmp.set('v.gridColumns', columns);
// data
var nestedData = [
{
"name": "123555",
"accountName": "Rewis Inc",
"employees": 3100,
"phone": "837-555-1212",
"accountOwner": "http://example.com/jane-doe",
"accountOwnerName": "Jane Doe",
"billingCity": "Phoeniz, AZ"
},
{
"name": "123556",
"accountName": "Acme Corporation",
"employees": 10000,
"phone": "837-555-1212",
"accountOwner": "http://example.com/john-doe",
"accountOwnerName": "John Doe",
"billingCity": "San Francisco, CA",
"_children": [
{
"name": "123556-A",
"accountName": "Acme Corporation (Bay Area)",
"employees": 3000,
"phone": "837-555-1212",
"accountOwner": "http://example.com/john-doe",
"accountOwnerName": "John Doe",
"billingCity": "New York, NY",
"_children": [
{
"name": "123556-A-A",
"accountName": "Acme Corporation (Oakland)",
"employees": 745,
"phone": "837-555-1212",
"accountOwner": "http://example.com/john-doe",
"accountOwnerName": "John Doe",
"billingCity": "New York, NY"
},
{
"name": "123556-A-B",
"accountName": "Acme Corporation (San Francisco)",
"employees": 578,
"phone": "837-555-1212",
"accountOwner": "http://example.com/jane-doe",
"accountOwnerName": "Jane Doe",
"billingCity": "Los Angeles, CA"
}
]
},
{
"name": "123556-B",
"accountName": "Acme Corporation (East)",
"employees": 430,
"phone": "837-555-1212",
"accountOwner": "http://example.com/john-doe",
"accountOwnerName": "John Doe",
"billingCity": "San Francisco, CA",
"_children": [
{
"name": "123556-B-A",
"accountName": "Acme Corporation (NY)",
"employees": 1210,
"phone": "837-555-1212",
"accountOwner": "http://example.com/jane-doe",
"accountOwnerName": "Jane Doe",
"billingCity": "New York, NY"
},
{
"name": "123556-B-B",
"accountName": "Acme Corporation (VA)",
"employees": 410,
"phone": "837-555-1212",
"accountOwner": "http://example.com/john-doe",
"accountOwnerName": "John Doe",
"billingCity": "New York, NY",
"_children": [
{
"name": "123556-B-B-A",
"accountName": "Allied Technologies",
"employees": 390,
"phone": "837-555-1212",
"accountOwner": "http://example.com/jane-doe",
"accountOwnerName": "Jane Doe",
"billingCity": "Los Angeles, CA",
"_children": [
{
"name": "123556-B-B-A-A",
"accountName": "Allied Technologies (UV)",
"employees": 270,
"phone": "837-555-1212",
"accountOwner": "http://example.com/john-doe",
"accountOwnerName": "John Doe",
"billingCity": "San Francisco, CA"
}
]
}
]
}
]
}
]
},
{
"name": "123557",
"accountName": "Rhode Enterprises",
"employees": 6000,
"phone": "837-555-1212",
"accountOwner": "http://example.com/john-doe",
"accountOwnerName": "John Doe",
"billingCity": "New York, NY",
"_children": [
{
"name": "123557-A",
"accountName": "Rhode Enterprises (UCA)",
"employees": 2540,
"phone": "837-555-1212",
"accountOwner": "http://example.com/john-doe",
"accountOwnerName": "John Doe",
"billingCity": "New York, NY"
}
]
},
{
"name": "123558",
"accountName": "Tech Labs",
"employees": 1856,
"phone": "837-555-1212",
"accountOwner": "http://example.com/john-doe",
"accountOwnerName": "John Doe",
"billingCity": "New York, NY",
"_children": [
{
"name": "123558-A",
"accountName": "Opportunity Resources Inc",
"employees": 1934,
"phone": "837-555-1212",
"accountOwner": "http://example.com/john-doe",
"accountOwnerName": "John Doe",
"billingCity": "Los Angeles, CA"
}
]
}
];
cmp.set('v.gridData', nestedData);
var expandedRows = ["123556", "123556-A", "123556-B", "123556-B-B", "123557"];
cmp.set('v.gridExpandedRows', expandedRows);
}
}); // eslint-disable-line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The data should be very similar. I would change _children
to nodes
align with Tree
component. Please review Tree data https://github.com/salesforce/design-system-react/blob/master/utilities/sample-data/tree/nodes-base.jsx#L2
}, | ||
{ | ||
id: '2', | ||
childOpen: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Tree
keys are:
* expanded: string,
* id: string,
* label: string or node,
* selected: string,
* type: string,
* nodes: array
}, | ||
{ | ||
label: 'Jane Doe', | ||
href: 'javascript:void(0);', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default cell render should be just a string. If you'd like to scope this project to only output strings I'm cool with that. We can add custom rendering later
DataTable
has a CustomCell
render that receives the tree data as props. The CustomCell
is the child of the Column child. See https://github.com/salesforce/design-system-react/blob/master/components/data-table/__examples__/advanced.jsx#L10
components/tree-grid/index.jsx
Outdated
/* Copyright (c) 2015-present, salesforce.com, inc. All rights reserved */ | ||
/* Licensed under BSD 3-Clause - see LICENSE.txt or git.io/sfdc-license */ | ||
|
||
// Implements the [Welcome Mat design pattern](https://lightningdesignsystem.com/components/welcome-mat/) in React. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo
components/tree-grid/index.jsx
Outdated
*/ | ||
id: PropTypes.string, | ||
|
||
labels: PropTypes.shape({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These aren't visible, so they go in assistiveText prop. Please see:
https://github.com/salesforce/design-system-react/blob/master/components/data-table/index.jsx#L55
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is still outstanding. ^
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, sorry, it's duplicate now. Please remove chooseRow
, selectAll
.
components/tree-grid/index.jsx
Outdated
'slds-table_bordered', | ||
'slds-table_edit', | ||
'slds-table_fixed-layout', | ||
'slds-table_resizable-cols', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless you are implementing resizable, this might need to be removed. I'd recommend not implementing it. It's not in the current Data either. Column width on the other hand is needed.
components/tree-grid/private/row.jsx
Outdated
) | ||
)} | ||
<td role="gridcell" style={{ width: '3.25rem' }}> | ||
<Button |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This component should accept a TreeRowActions
component as a child.
Typically, this component is going to have a fixed header, but if you want to descope that for now I'm OK with that. This comments means this is OK (no scrolling): http://design-system-react-components.herokuapp.com/?path=/story/sldsdatatable--advanced-fixed-layout but you don't have to do this: http://design-system-react-components.herokuapp.com/?path=/story/sldsdatatable--fixed-header |
I will try to get you a new props proposal by tomorrow, but feel free to ask questions tonight and maybe we can discuss tomorrow in the meeting. |
So far this is what I have:
|
b6118f1
to
a52fec1
Compare
Yes. That's correct. Please think of the selection as flat even if the data is hierarchal--therefore only use indeterminate on the "Select All" checkbox which is how DataTable works.
Yes. That's correct. There are some differing opinions on the select all interaction pattern. I'd would like to see Select All select all the rows in the table (even the hidden ones). Since updating the data would be handled in the example code anyway, if that pattern changes in the future, it's not a big deal--we'd just update the example--but it's good to provide a good default, because everyone loves to copy and paste! 😄 One way to think about this mash up of a DataTable and Tree here at Salesforce is that it's often a database union of a table with itself. That is take the ACCOUNTS table and merge it with same ACCOUNTS table based on COLUMN A which have the same value, so the data is visibly hierarchal, but you could also, flatten the data and add a column that "links the child/parent relationships." Another way to think of it is as file tree. Selection of a folder is not visible UI selection of the children. |
As for clicking the row, in the Tree there is selection and example in separate DOM nodes. What is the difference here? We need to keep single selection and simply a treegrid and not the merger of a DataTable and a TreeGrid that is the multiple selection variant. |
I am still unable to fix the issue of |
I have used event trap for selection and expansion handlers, and everything seems to be working fine now. In case of the checkbox, I had made the example exactly as you had said, so it didn't require any changes. I am yet to touch the switching part between Navigation Mode <-> Selection Mode. As far as I learnt: -
However, I have a slight confusion, regarding what objects will be focused I think we would be going with hovering each and every column in row (maybe except columns with some PS: I will start with browser testing later today, as rendering and functionality is almost done :) |
7990727
to
8ad82e2
Compare
This issue has been automatically marked as stale, because it has not had recent activity. It will be closed if no further activity occurs. Maintainers are responsible for tech debt and project health. This is most likely a new components or component feature request. Please submit a pull request for or request feedback on this feature. Thank you. |
This issue has been automatically marked as stale, because it has not had recent activity. It will be closed if no further activity occurs. Maintainers are responsible for tech debt and project health. This is most likely a new components or component feature request. Please submit a pull request for or request feedback on this feature. Thank you. |
This issue has been automatically marked as stale, because it has not had recent activity. It will be closed if no further activity occurs. Maintainers are responsible for tech debt and project health. This is most likely a new components or component feature request. Please submit a pull request for or request feedback on this feature. Thank you. |
Fixes #1836
CONTRIBUTOR checklist (do not remove)
Please complete for every pull request
npm run lint:fix
has been run and linting passes.components/component-docs.json
CI tests pass (npm test
).REVIEWER checklist (do not remove)
components/component-docs.json
tests.Required only if there are markup / UX changes
last-slds-markup-review
inpackage.json
and push.last-accessibility-review
, topackage.json
and push.npm run local-update
within locally cloned site repo to confirm the site will function correctly at the next release.