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

feat(datagrid): optimise layout rendering #1346

Merged
merged 4 commits into from
May 9, 2024
Merged

Conversation

dtsanevmw
Copy link
Contributor

@dtsanevmw dtsanevmw commented Apr 4, 2024

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)
  • If applicable, have a visual design approval

PR Type

What kind of change does this PR introduce?

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

What is the current behavior?

  • When computing header widths we get each header boundingClientRect and update the width separately which creates multiple reflows and slow us down.
  • Cells and Headers are listening for changes and holding column states.
  • Calculating userDefinedWidth triggers reflow each time when accessing getBoundingClientRect.
  • Expandable animation cycle is triggered as being changed initially which trigger setStyle.

Issue Number: CDE-1743

What is the new behavior?

  • All the widths are batched before traversing trough headers and their width being set.
  • Main renderer is handling the state listening and pass the changes to cells and headers to update.
  • We clone the target element before we calculate boundingClientRect outside the grid so we don't trigger reflow of the whole grid and just receive measurements for that specific element.
  • We set the expanded state to null to skip the animation cycle on initial load.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

Copy link
Contributor

github-actions bot commented Apr 4, 2024

👋 @dtsanevmw,

  • 🙏 The Clarity team thanks you for opening a pull request
  • 🎉 The build for this PR has succeeded
  • 🔍 The PR is now ready for review
  • 🍿 In the meantime, view a preview of this PR
  • 🖐 You can always follow up here. If you're a VMware employee, you can also reach us on our internal Clarity Support space

Thank you,

🤖 Clarity Release Bot

Copy link
Contributor

github-actions bot commented Apr 4, 2024

This PR introduces visual changes: bb7a261
If these changes are intended and correct, please cherry-pick the above commit to this PR.

git checkout dtsanevmw/cde-1743
git fetch https://github.com/vmware-clarity/ng-clarity.git bb7a261882230a2d8a1d1561ca7fb59b18f5185c
git cherry-pick bb7a261882230a2d8a1d1561ca7fb59b18f5185c
git push

@dtsanevmw dtsanevmw force-pushed the dtsanevmw/cde-1743 branch 2 times, most recently from e323b4c to cec3051 Compare April 8, 2024 10:06
Copy link
Contributor

github-actions bot commented Apr 8, 2024

This PR introduces visual changes: 20d8185
If these changes are intended and correct, please cherry-pick the above commit to this PR.

git checkout dtsanevmw/cde-1743
git fetch https://github.com/vmware-clarity/ng-clarity.git 20d818587944f33176b1d3ead208411a3d983b53
git cherry-pick 20d818587944f33176b1d3ead208411a3d983b53
git push

@dtsanevmw dtsanevmw force-pushed the dtsanevmw/cde-1743 branch 2 times, most recently from 1747eb3 to 009c48d Compare April 10, 2024 03:52
Copy link
Contributor

This PR introduces visual changes: 339d4ce
If these changes are intended and correct, please cherry-pick the above commit to this PR.

git checkout dtsanevmw/cde-1743
git fetch https://github.com/vmware-clarity/ng-clarity.git 339d4ce23a80bf845edd2660d3a9c632b0e430e4
git cherry-pick 339d4ce23a80bf845edd2660d3a9c632b0e430e4
git push

@dtsanevmw dtsanevmw marked this pull request as ready for review April 10, 2024 07:12
@dtsanevmw dtsanevmw requested a review from a team April 10, 2024 07:12
Copy link
Contributor

@valentin-mladenov valentin-mladenov left a comment

Choose a reason for hiding this comment

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

Check my comments.

Copy link
Contributor

This PR introduces visual changes: 1868bc5
If these changes are intended and correct, please cherry-pick the above commit to this PR.

git checkout dtsanevmw/cde-1743
git fetch https://github.com/vmware-clarity/ng-clarity.git 1868bc52e96bf0b2ee8fbfb4e9eed5b28bd93609
git cherry-pick 1868bc52e96bf0b2ee8fbfb4e9eed5b28bd93609
git push

Copy link
Contributor

@Jinnie Jinnie left a comment

Choose a reason for hiding this comment

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

Looks good. I will need some more time for a deeper review.
Posting some observations which we already discussed offline, but we want to have them logged in the PR.

projects/angular/src/data/datagrid/datagrid.spec.ts Outdated Show resolved Hide resolved
projects/angular/src/data/datagrid/datagrid-row.ts Outdated Show resolved Hide resolved
projects/angular/src/data/datagrid/render/main-renderer.ts Outdated Show resolved Hide resolved
@dtsanevmw dtsanevmw force-pushed the dtsanevmw/cde-1743 branch 2 times, most recently from b365a99 to 2cd3aeb Compare April 26, 2024 08:47
Copy link
Contributor

This PR introduces visual changes: d9d984b
If these changes are intended and correct, please cherry-pick the above commit to this PR.

git checkout dtsanevmw/cde-1743
git fetch https://github.com/vmware-clarity/ng-clarity.git d9d984b722d3712137c082a0a628ace05068fdb2
git cherry-pick d9d984b722d3712137c082a0a628ace05068fdb2
git push

@dtsanevmw dtsanevmw force-pushed the dtsanevmw/cde-1743 branch 2 times, most recently from 3c1ecd7 to 115688e Compare April 26, 2024 11:16
Copy link
Contributor

This PR introduces visual changes: 931dcf2
If these changes are intended and correct, please cherry-pick the above commit to this PR.

git checkout dtsanevmw/cde-1743
git fetch https://github.com/vmware-clarity/ng-clarity.git 931dcf24c3090378f96dfac4255fc2c78893d051
git cherry-pick 931dcf24c3090378f96dfac4255fc2c78893d051
git push

Copy link
Contributor

@valentin-mladenov valentin-mladenov left a comment

Choose a reason for hiding this comment

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

LGTM

@dtsanevmw dtsanevmw merged commit ac0d4cf into main May 9, 2024
8 checks passed
@dtsanevmw dtsanevmw deleted the dtsanevmw/cde-1743 branch May 9, 2024 07:27
Copy link
Contributor

github-actions bot commented May 9, 2024

The backport to 16.x failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-16.x 16.x
# Navigate to the new working tree
cd .worktrees/backport-16.x
# Create a new branch
git switch --create backport-1346-to-16.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 ac0d4cf1c8b6f9a9dc32ccc5b4b4f5445d49b781
# Push it to GitHub
git push --set-upstream origin backport-1346-to-16.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-16.x

Then, create a pull request where the base branch is 16.x and the compare/head branch is backport-1346-to-16.x.

dtsanevmw added a commit that referenced this pull request May 9, 2024
Please check if your PR fulfills the following requirements:

- [ ] Tests for the changes have been added (for bug fixes / features)
- [ ] Docs have been added / updated (for bug fixes / features)
- [ ] If applicable, have a visual design approval

What kind of change does this PR introduce?

<!-- Please check the one that applies to this PR using "x". -->

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

- When computing header widths we get each header boundingClientRect and
update the width separately which creates multiple reflows and slow us
down.
- Cells and Headers are listening for changes and holding column states.
- Calculating userDefinedWidth triggers reflow each time when accessing
getBoundingClientRect.
- Expandable animation cycle is triggered as being changed initially
which trigger setStyle.

<!-- Please describe the current behavior that you are modifying, or
link to a relevant issue. -->

Issue Number: CDE-1743

- All the widths are batched before traversing trough headers and their
width being set.
- Main renderer is handling the state listening and pass the changes to
cells and headers to update.
- We clone the target element before we calculate boundingClientRect
outside the grid so we don't trigger reflow of the whole grid and just
receive measurements for that specific element.
- We set the expanded state to null to skip the animation cycle on
initial load.

- [ ] Yes
- [X] No

<!-- If this PR contains a breaking change, please describe the impact
and migration path for existing applications below. -->

---------

Co-authored-by: GitHub <noreply@github.com>
dtsanevmw added a commit that referenced this pull request May 9, 2024
Please check if your PR fulfills the following requirements:

- [ ] Tests for the changes have been added (for bug fixes / features)
- [ ] Docs have been added / updated (for bug fixes / features)
- [ ] If applicable, have a visual design approval

What kind of change does this PR introduce?

<!-- Please check the one that applies to this PR using "x". -->

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

- When computing header widths we get each header boundingClientRect and
update the width separately which creates multiple reflows and slow us
down.
- Cells and Headers are listening for changes and holding column states.
- Calculating userDefinedWidth triggers reflow each time when accessing
getBoundingClientRect.
- Expandable animation cycle is triggered as being changed initially
which trigger setStyle.

<!-- Please describe the current behavior that you are modifying, or
link to a relevant issue. -->

Issue Number: CDE-1743

- All the widths are batched before traversing trough headers and their
width being set.
- Main renderer is handling the state listening and pass the changes to
cells and headers to update.
- We clone the target element before we calculate boundingClientRect
outside the grid so we don't trigger reflow of the whole grid and just
receive measurements for that specific element.
- We set the expanded state to null to skip the animation cycle on
initial load.

- [ ] Yes
- [X] No

<!-- If this PR contains a breaking change, please describe the impact
and migration path for existing applications below. -->

---------

Co-authored-by: GitHub <noreply@github.com>
Copy link
Contributor

Hi there 👋, this is an automated message. To help Clarity keep track of discussions, we automatically lock closed PRs after 14 days. Please look for another open issue or open a new issue with updated details and reference this one as necessary.

@github-actions github-actions bot locked and limited conversation to collaborators May 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants