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-Table-Column-header #835

Closed
wants to merge 18 commits into from

Conversation

sunilkotiyaibm
Copy link
Contributor

#135

Closes #

Summary

Allow resizing Table columns using mouse dragging

Change List (commits, features, bugs, etc)

src/components/DataTable/_data-table.scss
src/components/Table/TableHead/TableHead.jsx
src/components/Table/TableHead/TableHeader.js
src/components/Table/TableHead/state/sorting.js
src/components/Table/TableHead/tools/cells.js
src/components/Table/TableHead/tools/sorting.js
src/components/Table/Table.test.jsx
src/components/Table/TableHead/TableHead.test.jsx
src/components/Dashboard/snapshots/Dashboard.story.storyshot
src/components/TableCard/snapshots/TableCard.story.storyshot
src/components/Table/snapshots/Table.story.storyshot
jest.config.js

Acceptance Test (how to verify the PR)

Allow resizing table column, on mouse hover resizing icon should be displayed
On resizing column using resizing icon should be dragged towards left or right direction using mouse down event
on releasing the mouse column should be resized where mouse was released and this should reflect in all rows.

@netlify
Copy link

netlify bot commented Jan 17, 2020

Deploy preview for carbon-addons-iot-react ready!

Built with commit b97002e

https://deploy-preview-835--carbon-addons-iot-react.netlify.com

@sunilkotiyaibm
Copy link
Contributor Author

@davidicus please review this PR

@sunilkotiyaibm
Copy link
Contributor Author

Hi @sunilkotiyaibm. This is getting close. I am still seeing some inconsistencies in the preview link though.
image

@davidicus , Ok we will re-look on this.

@davidicus 2nd image issue fixed and delivered, 1st image issue not reproducible.
Please review.

@davidicus
Copy link
Collaborator

davidicus commented Jan 23, 2020


Defect is still present

@kotiyas
Copy link

kotiyas commented Jan 24, 2020

Defect is still present

@davidicus This behavior is due to Filter text, which is stopping resize process.
It will not be squeeze untill text box is exist. Here textbox have some fixed width because of that column will not be reseized, if you off the filter then will start resizing. Do we need to take care this filter issue in this story /feature ?
As discussed wiith @JordanWSmith15 , we should disable the resizing when filter is on

@kotiyas
Copy link

kotiyas commented Jan 24, 2020


Defect is still present

@davidicus we will look into this first image issue when height of header is increased and resize

@sunilkotiyaibm
Copy link
Contributor Author

@JordanWSmith15 @davidicus we have fixed the resizing issue in first image, for filter scenario we need to finalize the behavior (here we can disable the filter if user resize, filter boxes have its own width so it resize the column when it is active again. ) Please review

@davidicus
Copy link
Collaborator

@sunilkotiyaibm I am not sure if that interaction works. Resizing these filter inputs seems like a prime use case for this feature. @JordanWSmith15 please feel free to weigh in. I am going to have Bjorn, whose been assigned to this issue, look into possible work arounds.

@JordanWSmith15
Copy link
Collaborator

@sunilkotiyaibm I am not sure if that interaction works. Resizing these filter inputs seems like a prime use case for this feature. @JordanWSmith15 please feel free to weigh in. I am going to have Bjorn, whose been assigned to this issue, look into possible work arounds.

@davidicus Agree that it is likely a use case, but given the length of time this story has gone on, I think it's time to draw the line and handle further capabilities as an add-on enhancement story, weighed against other priorities. I would be fine disabling resizing while the filters are active if it meant this story taking more than a few more days.

@stuckless
Copy link
Contributor

@davidicus @sunilkotiyaibm @JordanWSmith15 Since we are talking able the filters... I also think that these filter components need to be customizable (like cell renderers and cell editors) since right now, if we have a date filter or a time filter, etc, we have no control over the user input component. I'm also not a fan of the fact that the filters when enabled causes the table to grow quite considerably. I think the expectation should be that the when a filter row is enabled the space that it gets is current column width. I always find it disconcerting when I enable the filters and I feel like Iv'e lost columns, but then I realize it's because the table is now horizontal scrolling.

@davidicus
Copy link
Collaborator

@davidicus @sunilkotiyaibm @JordanWSmith15 Since we are talking able the filters... I also think that these filter components need to be customizable (like cell renderers and cell editors) since right now, if we have a date filter or a time filter, etc, we have no control over the user input component. I'm also not a fan of the fact that the filters when enabled causes the table to grow quite considerably. I think the expectation should be that the when a filter row is enabled the space that it gets is current column width. I always find it disconcerting when I enable the filters and I feel like Iv'e lost columns, but then I realize it's because the table is now horizontal scrolling.

I agree. This is the behavior we are going with. Removing the min width and have these inputs be 100% of their cell size. Bjorn is pushing up some changes this morning with his fixes.

@stuckless , custom sort filters are now a thing! Unfortunately, it seems to be a feature for Stateful table which i know you do not use. I wonder if that can be moved over to the base Table. We will have to look into.
#843

// 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.

this needs to be translated, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know, I think that @sunilkotiyaibm copied this file from carbon to temporarily add a forwarding ref.

Copy link
Collaborator

Choose a reason for hiding this comment

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

These ref changes need to be put into a PR on the Carbon repo still. I will open an issue to track.

Copy link
Collaborator

Choose a reason for hiding this comment

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

These ref changes need to be put into a PR on the Carbon repo still. I will open an issue to track.

Issue created

Copy link
Contributor

@scottdickerson scottdickerson left a comment

Choose a reason for hiding this comment

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

I made a couple of requests on the feature

columnVar.index = index;
columnVar.startX = columnVar.element.offsetLeft - e.clientX;
columnVar.move = e.clientX;
document.onmouseup = mouseupCallback;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we scope these to the component?

Copy link
Contributor

Choose a reason for hiding this comment

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

looking into it

// 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
Collaborator

Choose a reason for hiding this comment

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

These ref changes need to be put into a PR on the Carbon repo still. I will open an issue to track.

@davidicus
Copy link
Collaborator

going to close in favor of #867

@sstone2423 sstone2423 closed this Feb 6, 2020
@scottdickerson scottdickerson deleted the 3715-Resize-Table-Column-header branch February 18, 2020 23:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants