feat(AnalyticalTable): introduce autoResize column feature (#3196)

[Papa-Santo]

A functionality by which the cell width gets auto adjusted when we double click the data-resizer columns. Implemented for text.

Thank you for your contribution! 👏

To get it merged faster, kindly review the checklist below:

Pull Request Checklist

• Reviewed the Contributing Guidelines
  • Especially the How to Contribute section
• Correct commit message style

[cla-assistant]

All committers have signed the CLA.

[coveralls]

Pull Request Test Coverage Report for Build 9203673320

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

• For more information on this, see Tracking coverage changes with pull request builds.
• To avoid this issue with future PRs, see these Recommended CI Configurations.
• For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

• 62 of 65 (95.38%) changed or added relevant lines in 5 files are covered.
• 1 unchanged line in 1 file lost coverage.
• Overall coverage increased (+0.07%) to 88.576%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
packages/main/src/components/AnalyticalTable/hooks/useAutoResize.tsx	48	51	94.12%

Files with Coverage Reduction	New Missed Lines	%
packages/charts/src/components/ColumnChart/ColumnChart.tsx	1	89.47%

Totals
Change from base Build 9172744096:	0.07%
Covered Lines:	5474
Relevant Lines:	6180

---

💛 - Coveralls

[Lukas742]

Hi @Papa-Santo

Thanks a lot for your contribution! :)

Below you can find the first findings of my review, I will most probably continue it at the start of next week:

• Since expandable rows are rendering a button/icon (depending on theme) inside a cell, autoResizeable isn't working properly for them. Please either exclude grouped tables, tree tables (isTreeTable: true) and tables with expandable subcomponents (renderRowSubComponent with subComponentsBehavior Expandable or IncludeHeightExpandable), or include the icon/button and the indentation in the calculation. If you decide to exclude these modes, please add a note about it to the property.

• Maybe you can reuse some logic of the useDynamicColumnWidths hook. (E.g. stringToPx) - This is not a requirement, using canvas is fine for me as well, I just wanted to point out that such a thing exists.

[Lukas742]

Hi @Papa-Santo

I really like the approach with a plugin hook. 👍
I still found some things that could be improved and I will create a commit for it, so you can take a look. I think this way it's easier for both of us, as explaining some of the changes probably takes more time than just showing them.

I plan to push the commit/s today.

[Papa-Santo]

Terrific. Thank you!

[Lukas742]

Hi @Papa-Santo

I've pushed my changes. I've also added some comments to explain why I changed some things.

I didn't have time to test these changes carefully, but at least all test cases are green :D

If you find any bugs or would do something different, please feel free to raise your concerns or adjust it directly.

[Papa-Santo]

This is a big improvement. I like how you were able to encapsulate everything in the hook. I was wanting to accomplish that same thing.

I only have a concern about the new measurements. Some of it might be due to my lack of understanding of isRtl.

Here is a bug that came up after these changes (See the behavior of resizing age):

Bug-Age.mp4

I have a fix that changes code inside of getMeasureMax and allows us to measure other types of elements as well:

Bug-Fix-Extra-Functionality.mp4

The code is pushed in.

[Lukas742]

Hi @Papa-Santo

thanks again for your input!

I think measuring children is the right approach here, so great idea :)
I've refactored the measure function again, so we don't have to rely on styles anymore (except for padding). With this approach no additional logic is necessary to support RTL reading direction. Also, custom content is now partially supported.

• querySelectors don't start at the document level now, but on "tabel-level"
• The largest cell is now retrieved by checking text span and only then each child of the cell is measured. For tree tables this would lead to wrong values depending on nesting levels, so if the cell containing the expand/collapse button is auto-resized each child of each visible cell is measured.
• The description and type of the public event were adjusted.

Looking forward to your feedback :)

[Papa-Santo]

I read through the code and tested for a bit now. It's working great! This works for all the normal use cases I tried and it is more efficient.

It is important to add what you added to the description. I did some ecotic table setups and came across a small regression.

• We lose support for conditional rendering of elements. I'll post two videos to show that.

With the current measuring:

Conditional-Rendering-Unsupported.mp4

With the measurements before changes:

Conditional-Rendering-Support.mp4

I really like this part of the solution:

• For tree tables this would lead to wrong values depending on nesting levels, so if the cell containing the expand/collapse button is auto-resized each child of each visible cell is measured.

If you agree that this solutions efficiency is better than the more costly solution which adds support for custom cells with conditional rendering, this seems ready.

Thanks again!

[Lukas742]

We lose support for conditional rendering of elements. I'll post two videos to show that.

Sorry, I didn't go into enough details for custom cell support. I think in the initial implementation we don't need to support that behavior, since we can't support every possible content passed to a cell anyway. If the need arises to also support basic custom content, we can then check to add a way to allow configuring the measuring method.

If you agree that this solutions efficiency is better than the more costly solution which adds support for custom cells with conditional rendering, this seems ready.

Yup, for the time being I'd like to keep this functionality :) I'll ask a colleague to have a quick look at this PR, and if he doesn't find anything we're good to merge. Thank you!

[ui5-webcomponents-react-bot]

🎉 This PR is included in version v1.29.0 🎉

The release is available on v1.29.0

Your semantic-release bot 📦🚀