Row z-index and a column resizer double click callback #189

jeremyk · 2017-07-05T17:44:12Z

Description

Row z-index change: we remove the style= z-index so that it can be set with CSS based on a selected row class.
We added a callback for double clicking the resizer
We added a showBottomBorder on all rows props.

Motivation and Context

Row z-index changes:

We wanted excel style drag down but the "handle" in the corner would be cut off by the row below. There was no easy way to fix that without making it so that the currently selected row would have a higher z-index.
-In the process I removed what seemed to be an unnecessary wrapping component: FixedDataTableRowImpl but I can put it back if it had a purpose I didn't understand. My general impression is that FDT2 has more of these than necessary so I started moving in that direction.

Column resizer: so could allow the column to auto size to its contents (like excel).

ShowBottomBorder: we couldn't get the bottom border on the last row in short tables to show.

How Has This Been Tested?

We are using this extensively in a private project that has a full QA team.

Screenshots (if appropriate):

Types of changes

Bug fix (non-breaking change which fixes an issue)
[x ] New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

[x ] My code follows the code style of this project.
[x ] My change requires a change to the documentation.
I have updated the documentation accordingly.
[x ] I have read the CONTRIBUTING document.

NOTE: I have not updated the documentation and I did commit to the dist folder. I'm not really sure how we could use this ourselves easily if I didn't do that (maybe someone can explain it to me?). There are also a couple other changes that probably need to be reverted for this to get reintegrated which I am happy to do but thought I would submit this PR first to just find out if these changes are wanted before I go to that trouble. Thanks.

Allows drag handle to naturally sit over row below.

Rows reusal causes order to not matter.

-Setting style is hard to override so use classes. -Remove wrapper that was preventing whole row z-index to be set based on selected row.

Use dist folder so we don't have to build internal.

Also, use better antialiasing method.

I had previously removed the row wrapper.

wcjordan · 2017-07-07T06:54:02Z

@KamranAsif could you take a look at this since you have the most extensive knowledge of issues related to row z-indexing? Also if you have some time I'd appreciate a primer on what we've dealt with in the past there.

wcjordan · 2017-07-10T20:49:20Z

dist/fixed-data-table-base.css

@@ -17,6 +17,107 @@
 * LICENSE file in the root directory of this source tree. An additional grant
 * of patent rights can be found in the PATENTS file in the same directory.
 *


Can you revert all the changes to dist & main.js?

wcjordan · 2017-07-10T20:52:30Z

site/base.less

@@ -6,7 +6,7 @@ html, body {
  -ms-text-size-adjust: 100%;
  margin: 0;
  padding: 0;
-  -webkit-font-smoothing: antialiased;
+  -webkit-font-smoothing: subpixel-antialiased;


what was your motivation here? could you revert unless there's a good driver?

I will revert

BTW, I was looking at this some more and I kind of think this should just be removed all together: http://usabilitypost.com/2012/11/05/stop-fixing-font-smoothing/

I think it's worth filing an issue for it, but I don't think it relates to your changeset and alters the styling of the docs so I'd rather not block merge on time to research it.

If I'm missing something and it's related to the changeset though, let me know.

wcjordan · 2017-07-10T20:53:19Z

src/FixedDataTableRow.js

@@ -106,6 +106,11 @@ class FixedDataTableRowImpl extends React.Component {
     * @param number distance
     */
    onColumnReorderEnd: PropTypes.func,
+
+    /**
+     * Whether the row has a bottom borer accross it


nit border spelling

wcjordan · 2017-07-10T20:55:12Z

src/FixedDataTableRow.js

-    FixedDataTableTranslateDOMPosition(style, 0, this.props.offsetTop, this._initialRender);
-
-    return (
-      <div


I'm not sure it's safe to remove this wrapping div. Could you add it back and we can revisit removing in a minor release.

Ok. I think I had a better reason but can't remember any more. I will put it back and see if my changes still work.

+1 please revert

Below, I noted that "the reason I removed the wrapping div is that the class needs to be added on the outer most div or it won't reorder the z-index of the row."

wcjordan · 2017-07-10T21:02:46Z

src/FixedDataTable.js

@@ -335,6 +345,7 @@ var FixedDataTable = createReactClass({
      showScrollbarX: true,
      showScrollbarY: true,
      touchScrollEnabled: false,
+      showBorderOnAllRows: true,


This should be false by default

Yep. Will do

wcjordan · 2017-07-10T21:06:42Z

src/FixedDataTable.js

@@ -709,8 +721,15 @@ var FixedDataTable = createReactClass({
    /*number|string*/ columnKey,
    /*object*/ event
  ) {
+
+    var nowMs = Date.now();
+    if (this.state.lastColumnResizeStart && nowMs - this.state.lastColumnResizeStart < 500) {


can you pull the < 500 into a double click timeout const in this file?

wcjordan · 2017-07-10T21:18:51Z

Kam's not available, so sorry for the wait. I've added some comments and think we can accept something like this (although I may strip it out in favor of #60 & #61).

I am seeing a lot of issues in the examples where headers & fixed columns are being overlapped by rows & scrollable columns when scrolling Y & X respectively. I'm also seeing an issue with the Resizable Columns example no longer working. Could you run npm run site-dev-server and fix the issues?

wcjordan · 2017-08-01T21:57:21Z

@jeremyk any update here? I'm going to cut a release shortly and wondering if I should wait on this.

jeremyk · 2017-08-01T22:19:24Z

Sorry, been on vacation and busy. The main update is that the reason I removed the wrapping div is that the class needs to be added on the outer most div or it won't reorder the z-index of the row. I thought I noted that here but it seems that I did not.

Anyway, if you are still interested I can get this cleaned up in a day - just let me know. Thanks!

wcjordan · 2017-08-01T22:25:29Z

Sure just depends on your priorities (there will always be more releases!). If you could give me an idea where you're at that will help me with deciding when to cut this weekend / Monday.

jeremyk · 2017-08-02T03:45:36Z

I will work on this tomorrow and if I don't finish I will give an update.

Pull request feedback.

jeremyk · 2017-08-02T22:56:50Z

Ok, I made all the requested changes except restoring the wrapper as I need that for my z-index change. I also merged from upstream and resolved the conflicts.

wcjordan · 2017-08-02T22:58:30Z

Great thanks. I'll give this a look soon and ship it out w/ a release this weekend or so.

Since I plan to do a minor version bump, the wrapper change should be less risky than doing it on a patch bump

KamranAsif

There is a lot going on here. Can you split this up into multiple reviews?

KamranAsif · 2017-08-10T23:50:28Z

src/FixedDataTable.js

+    var nowMs = Date.now();
+    if (this.state.lastColumnResizeStart && nowMs - this.state.lastColumnResizeStart < DOUBLE_CLICK_TIMEOUT) {
+      (this.props.onColumnResizeDoubleClick || emptyFunction)(columnKey);
+    }


Theres no reason to re-invent the wheel here. You should have a separate callback passed that uses the native onDoubleClick event handler

On a double click you were getting 2 regular resize callbacks. I wasn't trying to "reinvent the wheel" I just thought it easier to handle it all at this point.

KamranAsif · 2017-08-10T23:52:49Z

src/FixedDataTableColumnResizeHandle.js

@@ -141,9 +141,15 @@ var FixedDataTableColumnResizeHandle = createReactClass({
  },

  _onMove(/*number*/ deltaX) {
+
+    if (deltaX === 0) {
+      return;


Does this actually change anything?

It basically stopped the resize callbacks when they were essentially noops. So it doesn't really do anything but I think it makes for a cleaner api.

jeremyk · 2017-08-11T16:48:00Z

I can certainly switch to an ondblclick and clean it up some more but I don't really want to keep working on this pull request if you guys don't really want it. My motivation is that we have a fork going and these are all the things that we need to switch back to the non-forked version. I think that is better for everyone but I also don't really want to split it up for that reason.

wcjordan · 2017-08-11T19:10:52Z

We definitely do want to help merge you back to mainline, sorry if the feedback has come across as ungrateful.

The wrapping div issue may seem minor in your usage of FDT, but because FDT doesn't have a good system for styling components, a change like that can break the CSS styles of other users. So we need to carefully investigate if there are alternatives or hold off on a change like that until a major release like the upcoming redux one.

Also have you had a chance to look into the screenshot I posted?

jeremyk · 2017-08-11T20:35:37Z

Ok, hope I didn't come across as if I don't want to help because I do. But I was thrown a little off guard as you asked for a bunch of changes and I did them all and then a bunch more were asked for. Anyway, I did see your screenshot and I fixed the issues with that. I could possibly move the classes I need to the wrapper but one of my main thoughts on FDT is that it has too many wrappers (there are 3 divs around each row) that don't seem to have a purpose (maybe I just don't understand the purpose) so I was trying to help removing one. Also it is set up to add all the classes to the inner wrapper so it seemed like a bunch of work to change how the outer wrapper was set up for minimal gain. I am adding a couple screenshots to help visualize this change.

wcjordan · 2017-08-13T04:19:55Z

Totally agree that there are unnecessary wrappers on divs, and we'd like to clean them up. Unfortunately with libraries, even small things like that can break compatibility.

If you think you can make a pass at keeping it in place that will allow releasing this sooner. Then filing a follow up ticket to reduce the wrappers can be targeted for the v1 release.

Either way let me know where you end up and I can adjust so it gets merged to the right place.
Thanks!

wcjordan · 2020-05-04T17:05:28Z

Closing this PR as stale.

Jeremy Kallman and others added 12 commits February 22, 2017 12:16

Render out rows in reverse order.

43697eb

Allows drag handle to naturally sit over row below.

Revert row order change.

9a1430e

Rows reusal causes order to not matter.

A bunch of z-index related changes.

ff0f0ad

-Setting style is hard to override so use classes. -Remove wrapper that was preventing whole row z-index to be set based on selected row.

Fix missing bottom border bug.

89cf518

Update built files.

a169d6b

Use dist folder so we don't have to build internal.

Upating fixed data table to have borders after all rows.

bb265c8

Fixed data table - support double click on resize handle.

ddf746a

Fix eslint warnings due to package updates.

435b91d

Make font in cells in table look better on safari.

83df122

Also, use better antialiasing method.

Merge from upstream.

645fd0b

Fix merge.

d367c38

I had previously removed the row wrapper.

Merge subtrees from ventus fork

c6a2599

wcjordan requested changes Jul 10, 2017

View reviewed changes

Jeremy Kallman added 10 commits August 2, 2017 15:16

Merge changes from upstream

3361725

Add .idea to gitignore.

91ea2a4

Revert changes to dist files.

d3ebe9c

Revert font smoothing change.

c8b7890

Rever change to main.js

3057784

Make showBorderOnAllRows default to false.

87fd6a0

Pull request feedback.

Move 500ms for double click detection to const.

9d5c7a2

Fix typo: borer -> border.

3ba44ad

Fix z-index display issues.

7b4fcd4

Restore order or params for resize callback.

f95fa72

KamranAsif suggested changes Aug 10, 2017

View reviewed changes

KamranAsif reviewed Aug 10, 2017

View reviewed changes

Jeremy Kallman added 2 commits December 22, 2017 15:11

Merge branch 'master' of github.com:schrodinger/fixed-data-table-2

634bc61

Add className as this seems to be causing an error.

07eed9a

wcjordan closed this May 4, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Row z-index and a column resizer double click callback #189

Row z-index and a column resizer double click callback #189

jeremyk commented Jul 5, 2017

wcjordan commented Jul 7, 2017

wcjordan Jul 10, 2017

jeremyk Jul 10, 2017

wcjordan Jul 10, 2017

jeremyk Jul 10, 2017

jeremyk Aug 2, 2017

wcjordan Aug 2, 2017

wcjordan Jul 10, 2017

wcjordan Jul 10, 2017

jeremyk Jul 10, 2017

KamranAsif Aug 10, 2017

jeremyk Aug 11, 2017

wcjordan Jul 10, 2017

jeremyk Jul 10, 2017

wcjordan Jul 10, 2017

jeremyk Jul 10, 2017

wcjordan commented Jul 10, 2017

wcjordan commented Aug 1, 2017

jeremyk commented Aug 1, 2017

wcjordan commented Aug 1, 2017

jeremyk commented Aug 2, 2017

jeremyk commented Aug 2, 2017

wcjordan commented Aug 2, 2017

KamranAsif left a comment

KamranAsif Aug 10, 2017

jeremyk Aug 11, 2017

KamranAsif Aug 10, 2017

jeremyk Aug 11, 2017

jeremyk commented Aug 11, 2017 •

edited

Loading

wcjordan commented Aug 11, 2017

jeremyk commented Aug 11, 2017 •

edited

Loading

wcjordan commented Aug 13, 2017

wcjordan commented May 4, 2020

Row z-index and a column resizer double click callback #189

Row z-index and a column resizer double click callback #189

Conversation

jeremyk commented Jul 5, 2017

Description

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

wcjordan commented Jul 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wcjordan commented Jul 10, 2017

wcjordan commented Aug 1, 2017

jeremyk commented Aug 1, 2017

wcjordan commented Aug 1, 2017

jeremyk commented Aug 2, 2017

jeremyk commented Aug 2, 2017

wcjordan commented Aug 2, 2017

KamranAsif left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeremyk commented Aug 11, 2017 • edited Loading

wcjordan commented Aug 11, 2017

jeremyk commented Aug 11, 2017 • edited Loading

wcjordan commented Aug 13, 2017

wcjordan commented May 4, 2020

jeremyk commented Aug 11, 2017 •

edited

Loading

jeremyk commented Aug 11, 2017 •

edited

Loading