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

fix: draggable resizing col jumps to right #1421

Merged
merged 3 commits into from
Dec 6, 2018

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
Contributor

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
Contributor

@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
Contributor

@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
Contributor

@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
Contributor

@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  (adazzle#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 (adazzle#1421)

* fix: draggable resizing col jumps to right

* refactor: use ref instead of findDOMNode

* fix: accidentally removed cel

* Release 5.0.5 (adazzle#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