Removing FixedDataTableCell div #687

Archit101967 · 2023-06-28T06:59:10Z

Removing FixedDataTableCell div

Description

In this PR I have removed the div which was earlier used in FixedDataTableCell .Earlier the structure to render a cell was like FixedDataTableCell (div used for the cell container)->Type of Cell(Text cell,Date cell,Link cell,Image cell)->FixedDataTableCellDefault (div used for the text ) .But after the changes I have removed the div of FixedDataTableCell and the structure will be like FixedDataTableCell (no div used)->Type of Cell(Text cell,Date cell,Link cell,Image cell)->FixedDataTableCellDefault (div used for styling) .So I have provided all the styling and classnames to the FixedDataTableCell div only .And we have to also correct the resize and reorder cell rendering because their styling and functionalities will also get effected .

Motivation and Context

For optimising the performance of fixed-data-table-2

How Has This Been Tested?

Tested using our local examples

Screenshots (if appropriate):

Performance after removing that div

Performance before removing that div

Types of changes

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

Checklist:

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

AmanGupta2708 · 2023-06-28T19:07:19Z

package.json

@@ -11,6 +11,7 @@
    "@reduxjs/toolkit": "^1.5.1",
    "lodash": "^4.17.4",
    "prop-types": "^15.7.2",
+    "react-fps": "^1.0.6",


This is only supposed to be used for our internal testing, we should remove this.
Also there should not be any change in yarn.lock and package-lock.json should not exist at all.

AmanGupta2708 · 2023-06-28T19:08:17Z

yarn.lock

@@ -1033,26 +1033,10 @@
    "@jridgewell/resolve-uri" "^3.0.3"
    "@jridgewell/sourcemap-codec" "^1.4.10"

-"@nodelib/[email protected]":


There should not be any change in yarn.lock

AmanGupta2708 · 2023-06-28T19:10:51Z

package-lock.json

@@ -0,0 +1,12685 @@
+{


Delete this file as well, I think it's comes because you might have used npm to install some package but we should use yarn to add any package.

AmanGupta2708 · 2023-06-28T19:55:51Z

src/FixedDataTableCellDefault.js

-        {children}
-      </div>
-    );
+    return <CellDefaultComponent {...this.props} />;


FixedDataTableCellDefault, aka DataCell, is essentially a type of cell that clients can utilize as a cell renderer. Clients have the flexibility to use any valid element or function as a cell renderer, and we have no control over the styles they apply to them.

we want to solely apply the necessary styles to the content within the FixedDataTableCell. That content is what the client passes, which can be either a DataCell or any other customized cell, or a function that returns a cell, or simply raw data.

We are passing default styles and default classNames as props from FixedDataTableCell which can be used by client if he don't want to provide any styles to it .And if a client wants to add his own styles he can do that also

AmanGupta2708 · 2023-06-28T19:57:19Z

Can we group HOCs and legacy components in separate directory ?

AmanGupta2708 · 2023-06-28T19:58:43Z

examples/AutoScrollExample_changed.js

+      columnGroups: [],
+      columnsCount: 10000,
+    };
+    const cellRenderer = (props) => (


This example should also work without any change, even when not using legacy table.

Yes it will work

Doesn't seems to be working correctly for me.

Note that cell-render could be anything i.e DataCell, TextCell, LinkCell, ImageCell or any customized cell or raw data, and this example should render same table as before for each case.

Updated the auto scroll example

AmanGupta2708 · 2023-06-28T20:00:38Z

src/FixedDataCellGroupFunction.js

+import cx from './vendor_upstream/stubs/cx';
+
+function CellGroup(props) {
+  var style = {


here or every other place, use const instead, or let (only if you have to modify variable later).

src/FixedDataCellGroupLegacyFunction.js

AmanGupta2708 · 2023-06-28T20:05:46Z

src/FixedDataTable.js

@@ -872,6 +872,7 @@ class FixedDataTable extends React.Component {
          fixedColumnsWidth={this.props.fixedColumnsWidth}
          fixedRightColumnsWidth={this.props.fixedRightColumnsWidth}
          isGroupHeader={true}
+          shouldUseLegacyComponents={this.props.shouldUseLegacyComponents}


Declare proptype for shouldUseLegacyComponents above.

AmanGupta2708 · 2023-06-28T20:07:18Z

src/FixedDataTableContainer.js

@@ -30,6 +29,7 @@ class FixedDataTableContainer extends React.Component {
    defaultScrollbars: true,
    scrollbarXHeight: Scrollbar.SIZE,
    scrollbarYWidth: Scrollbar.SIZE,
+    shouldUseLegacyComponents: false,


@pradeepnschrodinger should it be true or false by default ??

This should be false by default to encourage users to NOT rely on legacy components.

AmanGupta2708 · 2023-06-28T20:10:28Z

src/css/layout/fixedDataTableCellLayout.css

@@ -59,7 +59,6 @@

 .fixedDataTableCellLayout/columnResizerContainer {
  position: absolute;
-  right: 0px;


why it removed ?

Because earlier we were placing the resizer knob from the right('0px') relative to the div we removed .So this knob was appearing at the end of that cell div .But now as we removed that div so I am adjusting the position of the knob from the left according to the width of each cell .The adjusting of position code is in ResizerKnobFunction where I am also providing left in the resizerKnobStyle

AmanGupta2708 · 2023-06-28T20:14:03Z

src/plugins/ResizeReorder/ResizeCell.js

@@ -64,12 +62,15 @@ class ResizeCell extends React.PureComponent {
          height={this.props.height}
          resizerLineHeight={this.context.tableHeight}
          onColumnResizeEnd={this.props.onColumnResizeEnd}
+          onColumnReorderEnd={this.props.onColumnReorderEnd}


why we need this here ?

To correct the functionality of resize and reorder cell combined example .Because in that example we are firstly rendering the Reorder Plugin and than it's child which is Resize plugin .So there is one more div which is being added which is the div of reorder cell and we are providing the styling in this div only which is also adjusting the cell from left .So, if our resize cell is being rendered after reorder cell than we have to adjust the left of resizer knob differently .The code of styling this resizer knob is in ResizerKnobFunction.js

AmanGupta2708 · 2023-06-28T20:16:17Z

src/plugins/ResizeReorder/ResizeCell.js

      touchEnabled,
      cellGroupType,
      onColumnReorderStart,
      ...props
    } = this.props;

    let style = {
-      height: props.height,
-      width: props.width - BORDER_WIDTH,
+      height: this.props.height,


we have already defined props you don't need to use this.props here and in next lines.

AmanGupta2708 · 2023-07-04T11:59:20Z

src/FixedDataTableCell.js

@@ -223,6 +225,9 @@ class FixedDataTableCell extends React.Component {
      height: this.props.height,
      width: this.props.width,
      left: this.props.left,
+      shouldUseLegacyComponents: this.props.shouldUseLegacyComponents,
+      style_default: style,


Here and every other place, use camelcase name.

AmanGupta2708 · 2023-07-04T12:06:45Z

src/FixedDataTableCell.js

@@ -286,13 +291,16 @@ class FixedDataTableCell extends React.Component {
        </FixedDataTableCellDefaultDeprecated>
      );
    }
-
-    const role = isHeaderOrFooter ? 'columnheader' : 'gridcell';


We should not remove role. keep it for legacy cell as it is, for new cell this could also be added in props.

AmanGupta2708 · 2023-07-04T12:11:31Z

src/plugins/ResizeReorder/ReorderCell.js

    };

+    if (!this.props.shouldUseLegacyComponents) {
+      if (left === undefined) style.display = 'none';


Please add a comment for why you do this ?
and what the reason for left going undefined here ?

Is it only the case with ReorderCell ? what about regular FixedDataTableCell ?

This was occuring only in reorder cell only because earlier I was setting left of style here(reorder cell) only but I can directly use the styleDefault in which I will fix the left in FixedDataTableCell and then pass the whole styleDefault as prop to reorder cell .So left will be set according to the rendering of FixedDataTableCell component .I corrected this thing in the new commit in other way and dropped this hacky way .

AmanGupta2708 · 2023-07-04T12:16:27Z

src/FixedDataTable.js

@@ -524,6 +524,8 @@ class FixedDataTable extends React.Component {
     * ```
     */
    rowAttributesGetter: PropTypes.func,
+
+    shouldUseLegacyComponents: PropTypes.bool,


Also write description for this.

AmanGupta2708 · 2023-07-04T12:23:02Z

src/hoc_components/FixedDataTableRowLegacyFunction.js

+
+function RowLegacy(props) {
+  const subRowHeight = props.subRowHeight || 0;
+  let style_wrapper = {


We can change the properties of const object as well, use const wherever you use let just because you later want to modify some of its properties.

AmanGupta2708 · 2023-07-04T12:24:41Z

yarn.lock

@@ -8418,4 +8418,4 @@ yargs@^11.0.0:
 yocto-queue@^0.1.0:
  version "0.1.0"
  resolved "https://registry.yarnpkg.com/yocto-queue/-/yocto-queue-0.1.0.tgz#0294eb3dee05028d31ee1a5fa2c556a6aaf10a1b"
-  integrity sha512-rVksvsnNCdJ/ohGc6xgPwyN8eheCxsiLM8mxuE/t/mOVqJewPuO1miLpTHQiRgTKCLexL4MeAFVagts7HmNZ2Q==


Why is this change ?

There is no use of this .

… way

Archit Dashora and others added 24 commits June 10, 2023 13:28

Impl

f7b8628

changes

c4e8c63

Remove cell group div checking of code for different examples is pending

fde920c

Final code after removing CellGroup div

f475a20

Choice on user whether to use new or old version

b90f33a

added if conditions at some places

4fe0bc9

Made legacy components for each component

4ff2935

Converted the code to use HOC components

51696af

Updated some part of the code

f4af336

after removing unnecessary changes

31fbf7e

Rebuild the node modules

5aabef5

react-fps added

d10313a

added CSS

a7f6949

Resize and reorder cell not fixed

9006cbd

some changes

f46247d

Fixed resize example

02ec021

working on reorder cell

8c90fec

working on dragging feature of reorder still remaining

ec226ff

container coming to right position

f6dc4a7

reorder cell resolved

77b730b

HOC components made

1fe34ad

updated the auto scroll example

eeef456

removed extra spaces

2cfdf0d

Updated the auto scroll example

44b9a96

AmanGupta2708 requested changes Jun 28, 2023

View reviewed changes

AmanGupta2708 reviewed Jun 28, 2023

View reviewed changes

Archit101967 added 2 commits June 29, 2023 11:44

removed package-lock.json and tools used for internal testing

71203f3

removed internal testing from autoscrollexample_changed.js

505204a

Archit101967 added 6 commits June 30, 2023 02:41

changed variable names and will fix autoscroll example

329a443

fixed auto scroll example

479796e

seperated the HOC components into different folder

31dc976

Checking performance

c419014

correcting reorder cell

1e5dfd8

corrected reorder cell rendering issue

57cdf03

AmanGupta2708 reviewed Jul 4, 2023

View reviewed changes

corrected variable names and also fixed reorder cell example in other…

a813f30

… way

pradeepnschrodinger mentioned this pull request Aug 3, 2023

Add docs for V2 #692

Merged

7 tasks

pradeepnschrodinger added the headless label Nov 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Removing FixedDataTableCell div #687

Removing FixedDataTableCell div #687

Archit101967 commented Jun 28, 2023

AmanGupta2708 Jun 28, 2023

AmanGupta2708 Jun 28, 2023

AmanGupta2708 Jun 28, 2023

AmanGupta2708 Jun 28, 2023 •

edited

Loading

Archit101967 Jun 30, 2023

AmanGupta2708 commented Jun 28, 2023

AmanGupta2708 Jun 28, 2023

Archit101967 Jun 29, 2023

AmanGupta2708 Jun 29, 2023

Archit101967 Jun 30, 2023

AmanGupta2708 Jun 28, 2023

AmanGupta2708 Jun 28, 2023

Archit101967 Jun 30, 2023

AmanGupta2708 Jun 28, 2023

pradeepnschrodinger Jun 29, 2023

AmanGupta2708 Jun 28, 2023

Archit101967 Jun 29, 2023

AmanGupta2708 Jun 28, 2023

Archit101967 Jun 29, 2023

AmanGupta2708 Jun 28, 2023

Archit101967 Jun 30, 2023

AmanGupta2708 Jul 4, 2023

Archit101967 Jul 4, 2023

AmanGupta2708 Jul 4, 2023

Archit101967 Jul 4, 2023

AmanGupta2708 Jul 4, 2023

AmanGupta2708 Jul 4, 2023

Archit101967 Jul 4, 2023 •

edited

Loading

AmanGupta2708 Jul 4, 2023

Archit101967 Jul 4, 2023

AmanGupta2708 Jul 4, 2023

Archit101967 Jul 4, 2023

AmanGupta2708 Jul 4, 2023

Archit101967 Jul 4, 2023

Removing FixedDataTableCell div #687

Are you sure you want to change the base?

Removing FixedDataTableCell div #687

Conversation

Archit101967 commented Jun 28, 2023

Description

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AmanGupta2708 Jun 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AmanGupta2708 commented Jun 28, 2023

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

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

Archit101967 Jul 4, 2023 • edited Loading

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

AmanGupta2708 Jun 28, 2023 •

edited

Loading

Archit101967 Jul 4, 2023 •

edited

Loading