Skip to content
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

feat(table): 3715-Resize-DataTable-Column #788

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
320 changes: 260 additions & 60 deletions src/components/Dashboard/__snapshots__/Dashboard.story.storyshot

Large diffs are not rendered by default.

20 changes: 20 additions & 0 deletions src/components/DataTable/_data-table.scss
Original file line number Diff line number Diff line change
Expand Up @@ -97,3 +97,23 @@ html[dir='rtl'] {
height: $spacing-09;
padding: $spacing-05;
}

.#{$prefix}--data-table th {
position: relative;
}
.column-resize-wrapper {
top: 0;
right: -$spacing-01;
width: $spacing-02;
cursor: col-resize;
height: 100%;
z-index: 1;
position: absolute;
user-select: none;
outline: none;
-moz-user-select: none;
Copy link
Collaborator

Choose a reason for hiding this comment

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

auto prefixer will take care of these browser prefixes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@davidicus This is custom class for resize component. So do we need to use prefix here?

#{$prefix}--column-resize-wrapper for className="bx--column-resize-wrapper"
Is it that you want.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@sunilkotiyaibm I am referring to line 114. These vender prefixes will be supplied by our autoprefixer plugin and do not have to be in source.

-webkit-user-select: none;
&:hover {
background-color: $ui-05;
}
}
61 changes: 56 additions & 5 deletions src/components/Table/TableHead/TableHead.jsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import React from 'react';
import React, { useState, useEffect, createRef } from 'react';
import PropTypes from 'prop-types';
import { DataTable, Checkbox } from 'carbon-components-react';
import isNil from 'lodash/isNil';
Expand All @@ -11,8 +11,9 @@ import { tableTranslateWithId } from '../../../utils/componentUtilityFunctions';

import ColumnHeaderRow from './ColumnHeaderRow/ColumnHeaderRow';
import FilterHeaderRow from './FilterHeaderRow/FilterHeaderRow';
import TableHeader from './TableHeader';

const { TableHead: CarbonTableHead, TableRow, TableExpandHeader, TableHeader } = DataTable;
const { TableHead: CarbonTableHead, TableRow, TableExpandHeader } = DataTable;

const propTypes = {
/** Important table options that the head needs to know about */
Expand Down Expand Up @@ -68,6 +69,7 @@ const propTypes = {
/** lightweight */
lightweight: PropTypes.bool,
i18n: I18NPropTypes,
hasResize: PropTypes.bool,
};

const defaultProps = {
Expand All @@ -82,6 +84,7 @@ const defaultProps = {
i18n: {
...defaultI18NPropTypes,
},
hasResize: true,
};

const StyledCheckboxTableHeader = styled(TableHeader)`
Expand Down Expand Up @@ -147,9 +150,52 @@ const TableHead = ({
closeMenuText,
lightweight,
i18n,
hasResize,
}) => {
const filterBarActive = activeBar === 'filter';

const [columnWidth, setColumnWidth] = useState({});
const columnRef = ordering.map(() => createRef());
const columnVar = {
index: 0,
element: Node,
startX: 0,
};
const mousemoveCallback = e => {
const leftColumn = columnRef[columnVar.index].current.getBoundingClientRect();
const rightColumn = columnRef[
columnVar.index
].current.nextElementSibling.getBoundingClientRect();
const mousePosition = e.clientX + columnVar.startX;
if (mousePosition >= 50 && mousePosition <= leftColumn.width + rightColumn.width - 50) {
columnVar.element.style.left = `${mousePosition}px`;
Copy link

Choose a reason for hiding this comment

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

columnVar.element.style.left = ${mousePosition}px;
Here we have implemented the approach which is suggested by Sean. Now column is rederring on release of mouse only.
Also we have implemented column dragging liner (the approach which we have shared for which Sean/Luke agreed )
in this implementation in table head we are setting liner (resize div) position through style directly because if we are using state then it start showing performance issue because of re-renderring. Please suggest on this.

} else {
document.onmousemove = null;
}
};
const mouseupCallback = () => {
document.onmouseup = null;
document.onmousemove = null;
const resizePosition = columnVar.element.offsetLeft + columnVar.element.clientWidth;
setColumnWidth(psd => ({
...psd,
[columnVar.index]: resizePosition,
[columnVar.index + 1]:
columnWidth[columnVar.index + 1] + (columnWidth[columnVar.index] - resizePosition),
}));
};
const onMouseUpCallback = (e, index) => {
columnVar.element = e.target;
columnVar.index = index;
columnVar.startX = columnVar.element.offsetLeft - e.clientX;
document.onmouseup = mouseupCallback;
document.onmousemove = mousemoveCallback;
};
useEffect(() => {
const nextWidth = columnRef.map(ref =>
ref.current ? ref.current.getBoundingClientRect().width : undefined
);
setColumnWidth(nextWidth);
}, []);
return (
<StyledCarbonTableHead lightweight={`${lightweight}`}>
<TableRow>
Expand All @@ -169,7 +215,7 @@ const TableHead = ({
</StyledCheckboxTableHeader>
) : null}

{ordering.map(item => {
{ordering.map((item, i) => {
const matchingColumnMeta = columns.find(column => column.id === item.columnId);
const hasSort = matchingColumnMeta && sort && sort.columnId === matchingColumnMeta.id;
const align =
Expand All @@ -181,7 +227,8 @@ const TableHead = ({
data-column={matchingColumnMeta.id}
isSortable={matchingColumnMeta.isSortable}
isSortHeader={hasSort}
width={matchingColumnMeta.width}
ref={columnRef[i]}
style={{ width: columnWidth[i] }}
onClick={() => {
if (matchingColumnMeta.isSortable && onChangeSort) {
onChangeSort(matchingColumnMeta.id);
Expand All @@ -195,6 +242,10 @@ const TableHead = ({
})}
>
<TableCellRenderer>{matchingColumnMeta.name}</TableCellRenderer>
{hasResize && i < ordering.length - 1 ? (
// eslint-disable-next-line jsx-a11y/no-static-element-interactions
<div className="column-resize-wrapper" onMouseDown={e => onMouseUpCallback(e, i)} />
) : null}
</StyledCustomTableHeader>
) : null;
})}
Expand Down
177 changes: 177 additions & 0 deletions src/components/Table/TableHead/TableHeader.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,177 @@
/**
* Copyright IBM Corp. 2016, 2018
*
* This source code is licensed under the Apache-2.0 license found in the
* LICENSE file in the root directory of this source tree.
*/

import cx from 'classnames';
import PropTypes from 'prop-types';
import React from 'react';
import { settings } from 'carbon-components';
import {
ArrowUp20 as Arrow,
ArrowsVertical20 as Arrows,
} from '@carbon/icons-react';

import { sortStates } from './state/sorting';
Copy link
Collaborator

@davidicus davidicus Jan 2, 2020

Choose a reason for hiding this comment

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

This file is going to have to be duplicated if we are going to be using this TableHeader copy. It currently breaks the Travis build as it can not be resolved. Final approver on this PR should make sure there is an issue open to remove these files on updating to the Carbon package version where these changes will be found

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we will create an issue open to remove or refactor the temporary changes for updated Carbon version.
Our objective for this draft PR is to get it reviewed at high level/major suggestions, not sure we can convert this PR as normal, I am going to create new PR once development is completed with all major suggestion included and smooth working of resizing column, currently we are able to resize but that is not smooth as expected.


const { prefix } = settings;

const translationKeys = {
iconDescription: 'carbon.table.header.icon.description',
};

const translateWithId = (key, { sortDirection, isSortHeader }) => {
if (key === translationKeys.iconDescription) {
if (isSortHeader) {
// When transitioning, we know that the sequence of states is as follows:
// NONE -> ASC -> DESC -> NONE
if (sortDirection === sortStates.NONE) {
return 'Sort rows by this header in ascending order';
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm really confused on how translation is supposed to work here? In many places the translation strings are defined in i8nproperty... but here we seem to have a singlekey` and then we use a complex set of if statements to turn that into hard coded text. Is this a translation pattern that is used somewhere else? I'm not sure how can translate this text? @davidicus @tay1orjones is a new iot translation pattern?

}
if (sortDirection === sortStates.ASC) {
return 'Sort rows by this header in descending order';
}

return 'Unsort rows by this header';
}
return 'Sort rows by this header in ascending order';
}

return '';
};

const sortDirections = {
[sortStates.NONE]: 'none',
[sortStates.ASC]: 'ascending',
[sortStates.DESC]: 'descending',
};

const TableHeader = React.forwardRef(function TableHeader(
{
className: headerClassName,
children,
isSortable,
isSortHeader,
onClick,
scope,
sortDirection,
translateWithId: t,
...rest
},
ref
) {
if (!isSortable) {
return (
<th {...rest} className={headerClassName} scope={scope} ref={ref}>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added ref property for reference when sorting is disabled.

Copy link
Collaborator

Choose a reason for hiding this comment

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

is this a change from Taylor's PR to Carbon? If so, we will want to make a PR for any other changes here so that we do not have to maintain this file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@davidicus this particular change is new, which also need to added in Carbon as earlier change for ref added by @tay1orjones, As carbon version for taylor is not updated in IOT, so he suggested to create/copy same file in IOT during development (or temporary).

Copy link
Contributor Author

@sunilkotiyaibm sunilkotiyaibm Jan 2, 2020

Choose a reason for hiding this comment

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

@davidicus Can you raise a PR in Carbon for this ref={ref} change line 67 and another change at line 88 for {...rest}.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@davidicus Can you raise a PR in Carbon for this ref={ref} change line 67 and another change at line 88 for {...rest}.

Please update

Copy link
Collaborator

@davidicus davidicus Jan 13, 2020

Choose a reason for hiding this comment

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

@davidicus Can you raise a PR in Carbon for this ref={ref} change line 67 and another change at line 88 for {...rest}.

Please update

I am not certain Carbon will allow us to spread props indiscriminately onto the th. They do it themselves for one of the branches though.

<span className={`${prefix}--table-header-label`}>{children}</span>
</th>
);
}

const className = cx(headerClassName, {
[`${prefix}--table-sort`]: true,
[`${prefix}--table-sort--active`]:
isSortHeader && sortDirection !== sortStates.NONE,
[`${prefix}--table-sort--ascending`]:
isSortHeader && sortDirection === sortStates.DESC,
});
const ariaSort = !isSortHeader ? 'none' : sortDirections[sortDirection];

return (
<th
scope={scope}
className={headerClassName}
aria-sort={ariaSort}
ref={ref}
{...rest} >
Copy link
Contributor Author

Choose a reason for hiding this comment

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

...rest property is moved in from inner wrapper, because style(width) was not applying on while resizing. This also need to be updated in Carbon component.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@davidicus Can you please deliver this Carbon changes, I think i don't have access on Carbon.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Carbon is an open source project. Everyone has access to open a PR. I will see about pushing this change up though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@davidicus @stuckless
I cloned the carbon branch (https://github.com/carbon-design-system/carbon) for this additional changes in TableHeader.js (adding ref for sorting and moving ...rest property from button to ).
So I did the changes in TableHeader.js and executed yarn and yarn test with snapshot update
Also created a issue #5047 in Carbon (carbon-design-system/carbon#5047)
and commited the changes in Branch: 3715-Datatable-Column-Resize
but unable to push the changes (git push origin 3715-Datatable-Column-Resize) due to access permission.
Error is : remote: Permission to carbon-design-system/carbon.git denied to sunilkotiyaibm.
Please help on this.
Looping @stuckless @JordanWSmith15

Copy link
Contributor Author

@sunilkotiyaibm sunilkotiyaibm Jan 15, 2020

Choose a reason for hiding this comment

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

@davidicus I cloned the carbon branch (https://github.com/carbon-design-system/carbon) for this additional changes in TableHeader.js (adding ref for sorting and moving ...rest property from button to ).
So I did the changes in TableHeader.js and executed yarn and yarn test with snapshot update
Also created a issue #5047 in Carbon (carbon-design-system/carbon#5047)
and commited the changes in Branch: 3715-Datatable-Column-Resize
but unable to push the changes (git push origin 3715-Datatable-Column-Resize) due to access permission.
Error is : remote: Permission to carbon-design-system/carbon.git denied to sunilkotiyaibm.
So I am not able to raise PR.
Please help on this.
Looping @stuckless @JordanWSmith15 @pdixit

Copy link
Collaborator

Choose a reason for hiding this comment

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

The Carbon repo utilizes the forked git flow. If you look at their contribution guidelines you can get guidance. It requires that you fork the Carbon repo and push your changes to your fork. Once this is done you will be able to open a PR on the upstream repository.

<button className={className} onClick={onClick} >
<span className={`${prefix}--table-header-label`}>{children}</span>
<Arrow
className={`${prefix}--table-sort__icon`}
aria-label={t('carbon.table.header.icon.description', {
header: children,
sortDirection,
isSortHeader,
sortStates,
})}
/>
<Arrows
className={`${prefix}--table-sort__icon-unsorted`}
aria-label={t('carbon.table.header.icon.description', {
header: children,
sortDirection,
isSortHeader,
sortStates,
})}
/>
</button>
</th>
);
});

TableHeader.propTypes = {
/**
* Specify an optional className to be applied to the container node
*/
className: PropTypes.string,

/**
* Pass in children that will be embedded in the table header label
*/
children: PropTypes.node,

/**
* Specify whether this header is one through which a user can sort the table
*/
isSortable: PropTypes.bool,

/**
* Specify whether this header is the header by which a table is being sorted
* by
*/
isSortHeader: PropTypes.bool,

/**
* Hook that is invoked when the header is clicked
*/
onClick: PropTypes.func,

/**
* Specify the scope of this table header. You can find more info about this
* attribute at the following URL:
* https://developer.mozilla.org/en-US/docs/Web/HTML/Element/th#attr-scope
*/
scope: PropTypes.string,

/**
* Specify which direction we are currently sorting by, should be one of DESC,
* NONE, or ASC.
*/
sortDirection: PropTypes.oneOf(Object.values(sortStates)),

/**
* Supply a method to translate internal strings with your i18n tool of
* choice. Translation keys are avabile on the `translationKeys` field for
* this component.
*/
translateWithId: PropTypes.func,
};

TableHeader.defaultProps = {
className:'',
children:'',
isSortHeader:false,
isSortable: false,
sortDirection:'',
onClick:'',
scope: 'col',
translateWithId,
};

TableHeader.translationKeys = Object.values(translationKeys);

TableHeader.displayName = 'TableHeader';

export default TableHeader;
Loading