petyosi / react-virtuoso

The most powerful virtual list component for React
https://virtuoso.dev
MIT License
5.13k stars 299 forks source link

fix: adjust the way used to set data-test-id for TableBody #1073

Closed ts1994tw closed 4 months ago

ts1994tw commented 4 months ago

Overview

I'm trying to integrate react-virtuoso with MUI table in my current project, also it is also based on testing-library: ByTestIdhttps://testing-library.com/docs/queries/bytestid/, when tried to use ByTestId and found it probably might be needed to change

Changes

Check

Screenshot 2024-04-17 at 14 55 41

Conclusion

If it seems like a good fix, it'd be cool to see it become part of the library. Thanks for your time, and kudos for your fantastic works in open-source 🙇

petyosi commented 4 months ago

I'm ok with the fix, by the time I used that my understanding of the testid attribute was insufficient. However, there's more to be done to make the fix uniform - there are several other data-test-id attributes in the table and in the virtuoso component, and the e2e tests are using an approach that looks like this:

      const stickyItem = document.querySelector('[data-test-id=virtuoso-top-item-list] > div') as HTMLElement

If you have the capacity, I'm happy to accept a larger PR that replaces all of the test attributes with the more conventional data-testid, but the e2e tests need to be updated too.

Hope this makes sense, thank you for the effort.

ts1994tw commented 4 months ago

@petyosi Thanks for your time on reviewing this PR.

I'm good with putting efforts on adjustments which are needed. But compared to my branch name here boren/fix-the-way-used-to-set-data-test-id-for-table, I would be happy to have this PR merged first and I will create another PR for any needed adjustments about data-testid soon.

If purpose of this PR is focused enough and you're also happy to have this PR merged first, I would be appreciated.

petyosi commented 4 months ago

Merged in https://github.com/petyosi/react-virtuoso/releases/tag/v4.7.9, due to the rebase GitHub did not pick this up.