8000 DataTable: changing size of cells does not work in every situation by stephanpelikan · Pull Request #7294 · grommet/grommet · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

DataTable: changing size of cells does not work in every situation #7294

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

DataTable: changing size of cells does not work in every situation #7294

wants to merge 2 commits into from

Conversation

stephanpelikan
Copy link
Contributor

What does this PR do?

In DataTable the height is calculated in code what does not work on resizing any column's content (see #7177) if there is another plain-column. Additionally, there are performance issues with resizing columns as I experienced and create an issue for (see #7284). This PR is a pure CSS solution for the problem which solves both issues.

Where should the reviewer start?

See section "Actual Behavior" of #7177.

What testing has been done on this PR?

We manually tested both situations: On initializing the table and on changing cells content. We use this as a fork already in production for two costumers who expiriences #7177 or #7284.

How should this be manually tested?

In "Steps to reproduce" of #7177 a sandbox is provided. To test correct sizing on table initialization one can set the default-value to true at https://codesandbox.io/p/sandbox/datatable-collapse-issue-2jq6d9?file=%2Findex.js%3A255%2C1-256%2C1. Additionally, one can remove the line
https://codesandbox.io/p/sandbox/datatable-collapse-issue-2jq6d9?file=%2Findex.js%3A310%2C1-311%2C1 to also test for non-plain columns.

The performance issue mentioned in #7284 I'm not able to reproduce. Actually, I figured out the root cause using the browsers profiler. Using grommet patched by this PR solved the problem.

Do Jest tests follow these best practices?

  • screen is used for querying.
  • The correct query is used. (Refer to this list of queries)
  • asFragment() is used for snapshot testing.

Any background context you want to provide?

The author of the initial implementation didn't see that there is a pure CSS solution (see comment in https://github.com/grommet/grommet/blob/master/src/js/components/TableCell/TableCell.js#L129). Meanwhile there were changes in the same section 10c46fd which should be tested. I updated the Sandbox to Grommet's current version and the intermediate change does not effect the behavior of this PR.

What are the relevant issues?

Screenshots (if appropriate)

No

Do the grommet docs need to be updated?

No

Should this PR be mentioned in the release notes?

As you wish

Is this change backwards compatible or is it a breaking change?

Was backward compatible before 10c46fd. If the underlying issue of this change #6086 is still solved with the PR in place, then it is confirmed that it still is backward compatible.

@stephanpelikan stephanpelikan mentioned this pull request Aug 14, 2024
Closed
3 tasks
@stephanpelikan stephanpelikan force-pushed the bugfix/issue_7177_updated-tests branch from 70b13c6 to 128509d Compare August 14, 2024 08:52
@stephanpelikan
Copy link
Contributor Author

This PR replaces #7283 since I rebased to master. Jest test fail although I updated snapshots successfully in this branch (which is based on master). I don't have any idea what is the reason for those errors reported. Can you give me a hint?

@jcfilben
Copy link
Collaborator

It looks like you have snapshot changes committed to this branch that are unrelated to the DataTable change

@stephanpelikan stephanpelikan force-pushed the bugfix/issue_7177_updated-tests branch from c9aa4cc to 9c43ca3 Compare August 20, 2024 05:27
@stephanpelikan
Copy link
Contributor Author

It looks like you have snapshot changes committed to this branch that are unrelated to the DataTable change

What I did:

  1. Checkout master
  2. Cherry-pick my commit (246f476)
  3. Run yarn test-update
  4. Run yarn test
  5. Commit the new snapshots.

That's it. I don't know why all these snapshots change and whether they should or not.

However, my next attempt:

  1. Upgrade Node to v22.6.0.
  2. Checkout most recent master
  3. Run yarn test-update to ensure getting the same snapshots - no changes created
  4. Cherry-pick my commit (246f476)
  5. Run yarn test-update - this time I got these changes: 
    modified:   src/js/components/Data/__tests__/__snapshots__/Data-test.tsx.snap
modified:   src/js/components/DataFilters/__tests__/__snapshots__/DataFilters-test.tsx.snap
modified:   src/js/components/DataTable/__tests__/__snapshots__/DataTable-test.tsx.snap
modified:   src/js/components/DataTableColumns/__tests__/__snapshots__/DataTableColumns-test.tsx.snap
modified:   src/js/components/Markdown/__tests__/__snapshots__/Markdown-test.js.snap
modified:   src/js/components/Table/__tests__/__snapshots__/Table-test.tsx.snap
  6. Run yarn test - succeeds
  7. Commit the new snapshots and push to Github

Again the tests fail in the Github-pipeline but succeed locally.

Actually, I don't know what I can do else. Since the same tests succeed locally I guess the problem is not up to my commit (246f476). Can you do the same procedure and check whether the snapshots your computer builds also works on Github?

@jcfilben
Copy link
Collaborator

I pulled down your branch locally, pulled in the latest changes from master, ran yarn and got the following snapshots failing. On a clean branch when I run the tests I am not getting snapshot failures

Screenshot 2024-08-23 at 3 36 14 PM

Another note, it looks like this change is causing issues with the DataTable/Mulltiple Pins story on Firefox.

This is how the story is displaying with the changes from this branch:
Screenshot 2024-08-23 at 3 30 35 PM

Here is the story normally:
Screenshot 2024-08-23 at 3 34 43 PM

@stephanpelikan stephanpelikan force-pushed the bugfix/issue_7177_updated-tests branch 2 times, most recently from 2cc2740 to b794ca9 Compare October 1, 2024 14:41
@stephanpelikan
Copy link
Contributor Author

Finally, I made it:

  1. I rebased the branch again on top of master.
  2. All tests passed locally and in CircleCI (although I did the same procedure as during the last attempts 🤷).
  3. I found a CSS solution to keep the pinned header/footer working (see UI Test): This style in combination with limiting to tbody instead of all rows.
  4. I incorporated a change of the cell content (which is the root cause for the issue DataTable: changing size of cells does not work in every situation #7177) into the infinite scrolling storybook as a hidden feature (see UI Test, just click the charts to replace it by a textual representation and click again to get back to the chart).

I'm looking forward to your response! 👀

@stephanpelikan
Copy link
Contributor Author

As the UI Tests show there are still problems to be solved. I will check that...

@stephanpelikan
Copy link
Contributor Author

@jcfilben: I could fix the unintended UI test changes. There is only one change left, caused by the intended adaption to bring the new behavior into a DataTable story.

@stephanpelikan stephanpelikan force-pushed the bugfix/issue_7177_updated-tests branch from 7518622 to 417920b Compare October 17, 2024 05:24
@stephanpelikan
Copy link
Contributor Author

@jcfilben: Again I rebased my changes because master changed causing conflicts. However, now there are changes in UI Tests (Grid, Grid show with item 77) which are imho not caused by changes. An external image is not loaded/rendered:
image
Running the story book on my local machine those stories are fine. How to deal with this?

@jcfilben
Copy link
Collaborator

@jcfilben: Again I rebased my changes because master changed causing conflicts. However, now there are changes in UI Tests (Grid, Grid show with item 77) which are imho not caused by changes. An external image is not loaded/rendered:
image
Running the story book on my local machine those stories are fine. How to deal with this?

The Grid and Grid with show 77 stories sometimes don't load the image quickly enough for the snapshot so don't worry about those ones changing. However, I don't think the infinite scroll snapshot should change

@stephanpelikan
Copy link
Contributor Author

However, I don't think the infinite scroll snapshot should change

OK. I reverted the changes for that story. Now it is proved that there is actually no change.

@stephanpelikan stephanpelikan force-pushed the bugfix/issue_7177_updated-tests branch from a736f90 to 51d65d3 Compare October 23, 2024 11:29
@stephanpelikan
Copy link
Contributor Author

rebased +1

@stephanpelikan
Replace calculating table height by CSS solution (grommet#7177)
d387ce6 
Signed-off-by: Stephan Pelikan <stephan.pelikan@phactum.at>
@stephanpelikan
Update test snapshots (grommet#7177)
d35561d 
Signed-off-by: Stephan Pelikan <stephan.pelikan@phactum.at>
@stephanpelikan stephanpelikan force-pushed the bugfix/issue_7177_updated-tests branch from 51d65d3 to d35561d Compare October 31, 2024 14:15
@stephanpelikan
Copy link
Contributor Author

rebased +1

@jcfilben: would you mind to review the last changes and maybe consider to merge them? 👍

@jcfilben
Copy link
Collaborator
jcfilben commented Nov 4, 2024

Hey @stephanpelikan it might take me a little longer to get to this PR, DataTable styling is an area we've had some odd issues come up in the past so I need to set aside a bit of time to look through the snapshots, storybook, and test on different browsers.

@stephanpelikan
Copy link
Contributor Author

@jcfilben I just wanted to ask whether there is already a time horizon for the PR?

@jcfilben
Copy link
Collaborator

Hey @stephanpelikan the grommet team will take a look at this Thursday this week

@jcfilben jcfilben added the PRty Biweekly PR reviews by grommet team with hoping of closing such PRs label Dec 19, 2024
@jcfilben
Copy link
Collaborator

Hey @stephanpelikan we took at this PR today a look and one thing that we noticed was without the useEffect to update the height in TableCell.js the div within a cell with plain="noPad" no longer takes up the height of the cell

This is the div without the useEffect
Screenshot 2024-12-19 at 2 19 10 PM

This is the div with the useEffect
Screenshot 2024-12-19 at 2 20 42 PM

In the PR that added plain="noPad" it seems like the intent of the prop is to remove the padding from the cell but ensure that the cell contents takes up the full space within the cell. It seems like this change may affect the way that the plain="noPad" prop was intended to function and cause unexpected side effects.

@stephanpelikan
Copy link
Contributor Author

@jcfilben OK, I'll check this in the christmas vacation to make it work as before 👍 . As far as I remember plain="noPad" is only internally used, so I would have to check what causes this parameter to be activated. Can you share the sample to reproduce this problem quickly for me? If not, then I will also be good but a sample would safe some time.

@jcfilben
Copy link
Collaborator

Sounds good! We just took the https://storybook.grommet.io/?path=/story/visualizations-datatable-onselect--on-select-data-table story and made the name in the first row long enough to wrap to see the issue.

I think plain="noPad" was originally internal only but somewhere along the way it did get added to the documentation https://v2.grommet.io/table#plain

@jcfilben jcfilben removed the PRty Biweekly PR reviews by grommet team with hoping of closing such PRs label Jan 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@stephanpelikan @jcfilben
0