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

Reduce dom elements - Approach #1 #680

Open
wants to merge 4 commits into
base: column-virtualization
Choose a base branch
from

Conversation

Archit101967
Copy link

Description

Remove three div from FixedDataTableCellGroup and FixedDataTableRow
-> The first div was the wrapper div for the Cell group .
-> The second div was the wrapper div for the Row .
-> The third div was for row layout

Motivation and Context

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

How Has This Been Tested?

-> Tested using our local examples and performance dev tools

Screenshots (if appropriate):

div_removal_code column_virtualization_code -> First screenshot is of div_removal
-> Second screenshot is of column_virtualization

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.

@@ -171,7 +171,7 @@
.exampleTitle {
margin-bottom: 0;
overflow: hidden;
white-space: nowrap;
white-space: normal;
Copy link
Author

Choose a reason for hiding this comment

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

Change this style so that the content should not overflow from a particular cell .The text wraps to the next line when needed.


class FixedDataTableCellGroupImpl extends React.Component {
class FixedDataTableCellGroup extends React.Component {
Copy link
Author

Choose a reason for hiding this comment

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

We will pass all the props from FixedDataTableRow to FixedDataTableCellGroup .So, I changed the name of FixedDataTableCellGroupImpl to FixedDataTableCellGroup

@@ -59,6 +59,8 @@ class FixedDataTableCellGroupImpl extends React.Component {

isHeaderOrFooter: PropTypes.bool,

offsetLeft: PropTypes.number,

Copy link
Author

Choose a reason for hiding this comment

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

To remove the wrapper div of CellGroup, we have to place the groups (fixed,scrollable,fixedright) according to the offsetLeft. Earlier we were seperating offsetLeft in below FixedDataTableCellGroup class component and adding one div to position the groups there .

this.props.offsetLeft === nextProps.offsetLeft
);
}

Copy link
Author

Choose a reason for hiding this comment

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

This LifeCycleMethod of React will check if there is changes in any of the following props than it will re-render the class component .

static defaultProps = /*object*/ {
left: 0,
offsetLeft: 0,
};

Copy link
Author

Choose a reason for hiding this comment

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

To place any particular cell we need these two thing one is left and the other one is offsetLeft . The left will be relative to the cell group and offsetLeft will be the position of that group from the left .

@@ -201,17 +221,29 @@ class FixedDataTableRowImpl extends React.Component {
var style = {
width: this.props.width,
height: this.props.height + subRowHeight,
zIndex: this.props.zIndex ? this.props.zIndex : 0,
position: 'absolute',
};
Copy link
Author

Choose a reason for hiding this comment

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

To differentiate the header or group header from the other rows we are using zIndex here .We will provide the styling for the row in this div only .

this._initialRender,
this.props.isRTL
);

Copy link
Author

Choose a reason for hiding this comment

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

FixedDataTableTranslateDOMPosition is used to set the position of the row from the top according to its offsetTop

this.props.className,
cx('fixedDataTableRowLayout/body'),
cx('fixedDataTableRowLayout/rowWrapper')
)}
Copy link
Author

Choose a reason for hiding this comment

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

We have to join the wrapper class with the class of this div .One more div was used for fixedDataTableRowLayout also .We can join that class also to maintain the styling and removing one div also .

@@ -15,7 +15,7 @@
overflow: hidden;
position: absolute;
top: 0;
white-space: nowrap;
white-space: normal;
}

Copy link
Author

Choose a reason for hiding this comment

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

Change this style so that the content should not overflow from a particular cell .The text wraps to the next line when needed.

);
}
}

export default FixedDataTableRow;
Copy link
Author

Choose a reason for hiding this comment

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

Now there is no need of this wrapper div .We have provided the styling and the className to the corresponding div .So we can remove this FixedDataTableRow class component and rename FixedDataTableRowImpl to FixedDataTableRow as we are passing all the props from FixedDataTable to FixedDataTableRow.

@pradeepnschrodinger pradeepnschrodinger changed the title Div removal Reduce dom elements - Approach #1 Jun 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants