feat(table): 3715-Resize-DataTable-Column #788
Conversation
|
Deploy preview for carbon-addons-iot-react ready! Built with commit 25e4ff1 https://deploy-preview-788--carbon-addons-iot-react.netlify.com |
There was a problem hiding this comment.
In addition to my comment below, something else to think about that we'll have to include is keyboard accessibility.
Once a user has focused on the move/resize handle, they should be able to use the ← and → arrow keys to expand or contract the column size. We might want to do some research on that approach or run it by the IBM accessibility team to ensure that's the appropriate experience.
| let currentElement = '<>'; | ||
| const onMouseMovecallback = e => { | ||
| if (isResizable) { | ||
| currentElement.style.width = `${startWidth + (e.pageX - startX)}px`; |
There was a problem hiding this comment.
The width shouldn't be set imperatively like this. We need to store the width of each TableHeader in state via an object keyed with the same key placed on TableHeader, column-${matchingColumnMeta.id}. This would be done in a useState/useEffect hook. Then we would update the state when the handler/resize slider moves.
Each TableHeader would have
style={{width: this.state.tableHeaders[{`column-${matchingColumnMeta.id}`}]}}`Unfortunately to do this, we need to access the width of the <th> in TableHeader via a ref in the useEffect hander for each TableHeader. This is currently not possible. Unfortunately the TableHeader component is provided by Carbon, we don't control the source, and it is a functional component. Function components do not accept the ref attribute. Carbon can forward the ref, which would enable us to then use the refs to do this appropriately.
I've put a PR in on Carbon to update TableHeader to forward it's ref.
Until that's been merged, published, and we update to that version we seem to be blocked. In the meantime we can temporarily completely reimplement TableHeader to forward the ref while we wait on the contribution to be accepted.
There was a problem hiding this comment.
@tay1orjones Initially we tried with react hooks (useEffect, useState) but we were not able to get the reference of specific column, and while setting/resizing the width, width was setting for all table columns.
So what we understand that you have added the forward ref changes in carbon and we are waiting for PR acceptance (which you raised) and Carbon-IOT version upgrade (with latest Carbon) before adding our table header resize changes.
Once version upgrade is done in carbon-iot then we will implement your suggestion of getting individual ref width using useEffect and storing the width of each TableHeader using useState and applying resized width (style={{width: this.state.tableHeaders[{column-${matchingColumnMeta.id}}]}}`) .
Please correct me if our understanding is not in sync.
There was a problem hiding this comment.
Yep, that's correct. In the meantime, you can reimplement/copy TableHeader from my carbon PR and use that for the time being. This way you're not blocked and can work through the rest of the implementation.
Depending on the timeline and our ability to update to the new version that carbon will release with my change, we may wait to merge your changes until we've updated. Another option would be to merge your changes with the reimplemented/copied TableHeader with a new issue created to refactor once we've updated the Carbon version.
There was a problem hiding this comment.
@davidicus PR #788 is updated for ref created in Carbon, here we are using ref for reference using react hooks as suggested by @tay1orjones .
In carbon a ref={ref}) is created/forward to use in Carbon-IOT, but we also observed that same is also missing if sorting is false on a column, which we have added in TableHeader.js, this also need to be added in Carbon.
Please review the code.
|
My Carbon PR has been merged, although the release candidate for v10.9.0 has already been released, so I would expect the change to be in v10.9.1, probably in early January. |
| ) { | ||
| if (!isSortable) { | ||
| return ( | ||
| <th {...rest} className={headerClassName} scope={scope} ref={ref}> |
There was a problem hiding this comment.
Added ref property for reference when sorting is disabled.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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).
There was a problem hiding this comment.
@davidicus Can you raise a PR in Carbon for this ref={ref} change line 67 and another change at line 88 for {...rest}.
There was a problem hiding this comment.
@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
There was a problem hiding this comment.
@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.
| className={headerClassName} | ||
| aria-sort={ariaSort} | ||
| ref={ref} | ||
| {...rest} > |
There was a problem hiding this comment.
...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.
There was a problem hiding this comment.
@davidicus Can you please deliver this Carbon changes, I think i don't have access on Carbon.
There was a problem hiding this comment.
Carbon is an open source project. Everyone has access to open a PR. I will see about pushing this change up though.
There was a problem hiding this comment.
@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
There was a problem hiding this comment.
@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
There was a problem hiding this comment.
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.
| .column-resize-wrapper { | ||
| top: 0; | ||
| right: -8px; | ||
| width: 10px; |
There was a problem hiding this comment.
can use the rem(10px) helper sass function to avoid absolute px values
There was a problem hiding this comment.
Ok, we will take care this rem(10px) change.
There was a problem hiding this comment.
replaced with carbon spacing variable
| z-index: 1; | ||
| position: absolute; | ||
| user-select: none; | ||
| -moz-user-select: none; |
There was a problem hiding this comment.
auto prefixer will take care of these browser prefixes
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
@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.
| -moz-user-select: none; | ||
| -webkit-user-select: none; | ||
| &:hover { | ||
| background-color: #34a9db; |
There was a problem hiding this comment.
What color is this? This should probably be a carbon token?
There was a problem hiding this comment.
Using carbon token now
| pressed: false, | ||
| index : 0 | ||
| } | ||
| const [state, setState] = useState({TableHeader:[]}); |
There was a problem hiding this comment.
this state and setState value should be indicative of what state you are changing. (e.g. columnWidth, setColumnWidth)
There was a problem hiding this comment.
Ok, we will change into meaningful name.
| }) => { | ||
| const filterBarActive = activeBar === 'filter'; | ||
|
|
||
| const utilsVar ={ |
There was a problem hiding this comment.
Please make these variables names more applicable to what values they hold. utilsVar is a bit too generic.
There was a problem hiding this comment.
Ok, we will rename this.
| utilsVar.pressed = true; | ||
| utilsVar.startX = e.clientX; | ||
| utilsVar.index = index; | ||
| document.addEventListener('mousemove', onMouseMovecallback, true); |
There was a problem hiding this comment.
Can we not use these react mouse events instead of adding listeners to the document. If we must put them here we should also be removing these event handlers when the component un-mounts.
| @@ -195,6 +233,11 @@ const TableHead = ({ | |||
| })} | |||
| > | |||
| <TableCellRenderer>{matchingColumnMeta.name}</TableCellRenderer> | |||
| {hasResize && i < ordering.length-1 ? ( | |||
| <div role="button" className="column-resize-wrapper" | |||
There was a problem hiding this comment.
This div is used as a button. Any reason we can't use a button element?
| ) { | ||
| if (!isSortable) { | ||
| return ( | ||
| <th {...rest} className={headerClassName} scope={scope} ref={ref}> |
There was a problem hiding this comment.
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.
| index : 0 | ||
| } | ||
| const [state, setState] = useState({TableHeader:[]}); | ||
| const elementsRef = useRef(ordering.map(() => createRef())); |
There was a problem hiding this comment.
I do not think we need to use useRef and createRef(). I think you can just map over and create like you are doing in this function passed to useRef
| const [state, setState] = useState({TableHeader:[]}); | ||
| const elementsRef = useRef(ordering.map(() => createRef())); | ||
| useEffect(() => { | ||
| const nextWidth = elementsRef.current.map( |
There was a problem hiding this comment.
These variable names need to be clearer here. The table is a huge component so we need to be explicit in our naming to really convey what it is we are trying to do and what values these variables hold.
| ArrowsVertical20 as Arrows, | ||
| } from '@carbon/icons-react'; | ||
|
|
||
| import { sortStates } from './state/sorting'; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
| ].current.nextElementSibling.getBoundingClientRect(); | ||
| const mousePosition = e.clientX + columnVar.startX; | ||
| if (mousePosition >= 50 && mousePosition <= leftColumn.width + rightColumn.width - 50) { | ||
| columnVar.element.style.left = `${mousePosition}px`; |
There was a problem hiding this comment.
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.
|
@tay1orjones @davidicus How come the branches are not deployed? I'd like to actually interact with this before formally approving it. What needs to happen to get branches deployed? |
|
When clicking on the resize edge, which gives me the resize mouse pointer, it also triggers a change in the sorting for that column. |
There is currently a reference to a file that is not present that is breaking this build. @sunilkotiyaibm you can just copy that small object that There are some issues still in the implementation that would have to be addressed before we could merge as well, like resizing causing a change in the sort. |
|
There are some misalignments happening with the placement of resize bar as seen below. This discrepancy in placement is only shown when you happen to hover over the misplaced handle. For the header it is on it seems as if you can not resize the cell.
|
|
@davidicus we are getting (yarn jest -u) unit test failure, It seems this is because of carbon TableHeader.js file and not getting any clue to fix this. Please suggest on this, we are not able to commit in new branch. |
@davidicus we have fixed this issue locally. |
@davidicus we have fixed this sorting issue locally. |
| // 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'; |
There was a problem hiding this comment.
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?
|
Closing this draft PR, this will be tracking from new PR 835 (#835). |
#135
Closes #
Summary
Resize data table columns
Change List (commits, features, bugs, etc)
src/components/DataTable/_data-table.scss
src/components/Table/TableHead/TableHead.jsx
Acceptance Test (how to verify the PR)