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

Add sortedRow and sortDirection to Table Props in order to give the possibility to define initial sort order #1377

Closed
AndreeWille opened this issue Jan 24, 2022 · 3 comments

Comments

@AndreeWille
Copy link
Contributor

Component to amend

Table

Context

It would really help if it would be possible to specify the sortDirection and sortedRow as Table props. This would help i in case an action of the table changes the State of the outer Component and results in re rendering the Table component. When the Table was sorted then it loses the sortOrder and renders with sortOrder = undefined.

I would be happy to provide a pull request.

@robinmetral
Copy link
Contributor

Just to clarify: is this mostly intended as a workaround for #1291, where is we move the sorting logic from internal state to props, we could avoid the re-renders? Or are there other use cases for it?

If this is to solve the re-renders, I have some doubts since this essentially makes the Table component's API more complex without clear benefits (beyond the performance improvements, granted). I'd probably like to see if there's a way to solve this without changing the Table API.

If there's other use cases to the new props, I think this would be a very welcome addition, and in this case I just wanted to add a couple of thoughts:

  • were you thinking of adding the new props as a non-breaking change (i.e. maintaining the internal sorting functionality) or replacing the current state-based behavior with the props? Non-breaking can be released faster and avoids us having to migrate, but it also duplicates the sorting mechanism (it would be doable both in userland with the new props and internally with the current state-based logic). If we go for a breaking change, we need to question if the props-based API really is better than the current one where most sorting is abstracted away (I'd love to hear thoughts here)
  • bear in mind that the Table is on the roadmap to be rebuilt entirely—so if this is a lot of effort (both for implementation and migration), we might want to postpone it and keep use this learning to inform the next version's API. Not a problem to put this in a minor before the redesigned Table, though

@AndreeWille
Copy link
Contributor Author

Thank you @robinmetral for raising this question. This is not related to #1291 but rather a new feature thats makes it possible to either
set initial sort direction and the the sorted row on page load e.g sort the 3rd column descending when the table gets rendered
or
keep the sorting direction and sorted column on re render.

The second one is what would fix a bug in the item catalog where the sort direction gets lost on opening the variant list because opening the variant list changes the state in the parent component which then re renders the table with the initial "no sorting".

I think this is just a minor change (and i guess non breaking) and i have the change already ready in a branch. I will submit a pull request this makes it probably easier to understand what this change is about.

@robinmetral
Copy link
Contributor

Ah makes sense, thank you for the extra context! Thanks for the PR, will review now 🙂

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

No branches or pull requests

3 participants