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

Added new logic for resultsetTable to manage sorting and adding/removing rows properly #1938

Merged
merged 24 commits into from
May 9, 2024

Conversation

Mil4n0r
Copy link
Collaborator

@Mil4n0r Mil4n0r commented Apr 3, 2024

Checklist
(Check off all the items before submitting)

  • Build process is done without errors. All tests pass in the /lib directory.
  • Self-reviewed the code before submitting.
  • Meets accessibility standards.
  • Added/updated documentation to /website as needed.
  • Added/updated tests as needed.

Description
When interacting with Checkbox or any other changeable element, the page would be set to 1 no matter what the situation were. I have implemented a new logic in order to track changes when adding or removing rows.

I still haven't changed the tests because I want the new logic to be validated first, I also have doubts about how to handle both add/remove rows and cases where the data changes entirely (this last one not being taken into account right now).

Closes #1936

@jsuarezgonz jsuarezgonz self-assigned this Apr 5, 2024
@Mil4n0r
Copy link
Collaborator Author

Mil4n0r commented Apr 9, 2024

Closed for now until the topic is discussed.

@Mil4n0r Mil4n0r closed this Apr 9, 2024
@Mil4n0r Mil4n0r added the help wanted Extra attention is needed label Apr 17, 2024
@Mil4n0r
Copy link
Collaborator Author

Mil4n0r commented Apr 18, 2024

Reopened, will remove it from Draft when I receive more feedback in the issue.

@Mil4n0r Mil4n0r reopened this Apr 18, 2024
@Mil4n0r Mil4n0r removed the help wanted Extra attention is needed label Apr 18, 2024
@Mil4n0r
Copy link
Collaborator Author

Mil4n0r commented May 3, 2024

The new problem shared by Alan is caused by the fact that we are currently using index from the map to set the keys. That is a problem that I detected while working in the code quality PR and is pending to be fixed, as it is considered a bad practice in React (Specially for sortable data).

This would lead to 2 possible solutions:

  • Leaving the structure as it is but setting the keys with a randomly generated Id (such as useId or uuidv4), this is not the best solution because it is forced to generate new numbers each time that the data changes or is sorted.
  • Changing the structure to ensure that each row has a unique id (either adding a field to the data structure and telling users to make sure that it is not repeated or using the current sortValue and force it to be unique).

@Mil4n0r Mil4n0r marked this pull request as ready for review May 7, 2024 13:34
@Mil4n0r
Copy link
Collaborator Author

Mil4n0r commented May 7, 2024

I have added a new mechanism to generate keys prior to making the sorting. Generate an intermediate structure with separate ids and cells preventing us from having to change the rows prop structure.

I have done testing in local to ensure that it was working properly with a similar example than the one Alan shared (using ReactHookForm).

After we merge this, I will talk to him and ensure that it is working as expected for their specific use case.

@Mil4n0r Mil4n0r marked this pull request as draft May 7, 2024 14:50
@Mil4n0r Mil4n0r marked this pull request as ready for review May 8, 2024 06:40
@Mil4n0r Mil4n0r changed the title Added new logic for resultsetTable when changing rows Added new logic for resultsetTable to manage sorting and adding/removing rows properly May 8, 2024
@Jialecl Jialecl merged commit f319e71 into master May 9, 2024
4 checks passed
@Jialecl Jialecl deleted the Mil4n0r/resultsettable_checkbox-fix branch May 9, 2024 10:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ResultsetTable returns to the first page when re-rendered.
3 participants