Skip to content

Conversation

qili26
Copy link
Contributor

@qili26 qili26 commented Dec 5, 2018

Description

A few sentences describing the overall goals of the pull request's commits.

fix: draggable + resizable column issue for resizing
If a column is both draggable and resizable, when resizing the right border of the current column jumps to the right.

Please check if the PR fulfills these requirements

What kind of change does this PR introduce? (check one with "x")

[x] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Other... Please describe:

What is the current behavior? (You can also link to an open issue here)

What is the new behavior?

Does this PR introduce a breaking change? (check one with "x")

[ ] Yes
[x] No

If this PR contains a breaking change, please describe the impact and migration path for existing applications: ...

Other information:
before fix:
draggable resizing issue

after fix:
draggable resizing issue after fix

const right = e.pageX || (e.touches && e.touches[0] && e.touches[0].pageX) || (e.changedTouches && e.changedTouches[e.changedTouches.length - 1].pageX);
// if headerDom is a draggable div, the first element (which is the only element as well) is the actual column header div with the position info
const headerDom = ReactDOM.findDOMNode(this);
const left = headerDom.draggable ? headerDom.firstChild.getBoundingClientRect().left : headerDom.getBoundingClientRect().left;
Copy link
Collaborator

Choose a reason for hiding this comment

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

From what I understand, we need to access the cell. Is that correct? In yes then I think we should add a ref to the div and just use it directly. Another benefit is that React.findDOMNode will be deprecated soon so it preferred not to use it.

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, in brief, we should get the actual cell position, I will make this update.

Copy link
Collaborator

@amanmahajan7 amanmahajan7 left a comment

Choose a reason for hiding this comment

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

LGTM. Let's wait for one more review

Copy link
Collaborator

@nstepien nstepien left a comment

Choose a reason for hiding this comment

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

Do we actually need a ref for this? I feel like using e.target.parentNode should be enough to get the cell's div.

@amanmahajan7
Copy link
Collaborator

@MayhemYDG this makes sense but recently we have moved around a lot for components to fix a few issues and this may break the parent/child assumption. I would like to revisit HeaderCell code again so we can improve the composition something like DraggableHeaderCell(SortableHeaderCell(SimpleHeaderCell or column.headerRenderer)) and in that case setting a ref would probably be safer. WDYT?

Copy link
Collaborator

@nstepien nstepien left a comment

Choose a reason for hiding this comment

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

Sounds good to me.

@amanmahajan7 amanmahajan7 merged commit db9c770 into v5-patch Dec 6, 2018
@amanmahajan7 amanmahajan7 deleted the ql-v5-draggable-resizing-fix branch December 6, 2018 16:37
amanmahajan7 pushed a commit that referenced this pull request Dec 7, 2018
* Cell Tooltip - Focus Issues  (#1422)

* change level tooltip gets rendered and level it gets triggered

* remove unsed editors and fix imports.

* re order inclusion of frozen cells to avoid z-index css.

* address comments

* Revert "remove unsed editors and fix imports."

This reverts commit 3b22f60.

* fix issues

* fix issues

* remove extra space

* fix: draggable resizing col jumps to right (#1421)

* fix: draggable resizing col jumps to right

* refactor: use ref instead of findDOMNode

* fix: accidentally removed cel

* Release 5.0.5 (#1424)

* update changelog

* Version bump to 5.0.5 [ci skip]

* revert auto file changes

* address aman comments
rossjp pushed a commit to rossjp/react-data-grid that referenced this pull request Dec 14, 2018
* Cell Tooltip - Focus Issues  (Comcast#1422)

* change level tooltip gets rendered and level it gets triggered

* remove unsed editors and fix imports.

* re order inclusion of frozen cells to avoid z-index css.

* address comments

* Revert "remove unsed editors and fix imports."

This reverts commit 3b22f60.

* fix issues

* fix issues

* remove extra space

* fix: draggable resizing col jumps to right (Comcast#1421)

* fix: draggable resizing col jumps to right

* refactor: use ref instead of findDOMNode

* fix: accidentally removed cel

* Release 5.0.5 (Comcast#1424)

* update changelog

* Version bump to 5.0.5 [ci skip]

* revert auto file changes

* address aman comments
@stale
Copy link

stale bot commented Jan 5, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Please reopen this if you feel it has been incorrectly closed and we will do our best to look into it. Thank you for your contributions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants