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

[Proposal]: Add default sort to KTable #772

Closed
BabyElias opened this issue Sep 4, 2024 · 7 comments
Closed

[Proposal]: Add default sort to KTable #772

BabyElias opened this issue Sep 4, 2024 · 7 comments
Labels

Comments

@BabyElias
Copy link
Contributor

BabyElias commented Sep 4, 2024

Product

KDS

Description :

We plan to include a feature in KTable that will allow the table to be sorted by default based on any column when initially loaded.
For eg :
Frame 572

Desired behavior

In order to implement this, we propose two ways of configuring the API. Not sure, which one would actually be more suited from a user perspective :

  • Defining default sorter state with KTable itself, something like :
<KTable
  :headers="headers"
  :rows="rows"
  caption="Table with built-in sorting"
  sortable
  :default="{column: 'Full Name', state:'asc'}"
/>
  • Defining default sorter state when defining headers , something like :
<KTable
  :headers="headers"
  :rows="rows"
  caption="Table with built-in sorting"
  sortable
/>
data() {
  return {
    headers: [
      { label: 'Full Name', dataType: 'string', default : SORT_ASC },
      { label: 'Age', dataType: 'numeric' },
      { label: 'City', dataType: 'string' },
    ],
}
@MisRob MisRob added type: proposal New feature or request Component: KTable labels Sep 4, 2024
@MisRob
Copy link
Member

MisRob commented Sep 5, 2024

Thanks @BabyElias!

(1) When I looked at default with newcomer's eyes, it wasn't very clear to me what it's supposed to do. So I think we could improve the name. Perhaps defaultSortOrder, sortOrder, defaultSort?

(2) Imagining using :default="{column: 'Full Name', state:'asc'}" to me didn't feel very intuitive compared to configuring right on headers. Probably because we already have various configurations on headers, such as width, so the latter option fits that mental picture and feels cohesive.

Putting it together, maybe something like

data() {
  return {
    headers: [
      { label: 'Full Name', dataType: 'string', defaultSort : SORT_ASC },
      { label: 'Age', dataType: 'numeric' },
      { label: 'City', dataType: 'string' },
    ],
}

???

This is not meant to be conclusion. Just feedback based on my first thoughts - we can build on top of it or perhaps it will help to spark some alternative ideas. Glad to discuss. Let's see if we can also hear from others.

@BabyElias
Copy link
Contributor Author

The second approach using headers has also been my personal preference. It's more intuitive and easier to configure that way.

@AlexVelezLl
Copy link
Member

AlexVelezLl commented Sep 9, 2024

Something against the second option is What happen if the consumer sends multiple headers with the defaultSort key? I guess we will need to validate this prop against this, but the validation of the headers prop is becoming a little bit complex. And we will need to add more text to explain this in the api docs table. Also, what happen if in the future we need to allow sorting by multiple columns? we would need to add another key that indicates the priority of each sort (to know which is the key we will sort first, which second, etc), and then we would need to validate that these priorities are all different for every column. I dont think that we have plans for multi-column sorting right now, but if we end up with an extensible api, it will be easier to include it if we ever want to do it.

For this, having a separate prop sets the right restrictions, as we can set just one column to be sorted, and if in the future we want to include sorting by multiple columns, we will need just to change this prop from expecting an object to expecting an array of objects like [key1 ASC, key2 DESC, key3 ASC] (similar to the orderBy statemnt in SQL), where its easier to identify which column has more priority, and is less prominent to errors.

Something that could have more chance of happening is what if in the future we would want to store the sorting preferences of the user? In that case we would need to store an object similar to the one in the first option, so having this key like that will be easier to just pass this stored object in that case.

Something against of the first option (although we already kind of dicuss about it) is that we will need to have an id prop for headers, to clearly identify which column we refer to, but I think this is also something that we will probably need to do anyways as is very useful for several situations.

With this, I would have a slight preference towards the first option but with some modifications to something like:

<KTable
  :headers="headers"
  :rows="rows"
  caption="Table with built-in sorting"
  sortable
  :defaultSort="{ headerId: 'fullName', sort: 'asc' }"
/>

This is not mean to be a conclusion either, I would like to know what you think about the approach and see the advantages and disadvantages of each pattern and decide based on that.

@MisRob
Copy link
Member

MisRob commented Sep 10, 2024

Thank you @AlexVelezLl, I think these are too good considerations.

@MisRob
Copy link
Member

MisRob commented Sep 10, 2024

Another idea - over time we will also see better what other configuration options we need to add to the table (there's potential for many) and consider if configuring via possible useKTable composable could be interesting.

Based on the feedback from the team, I'm preparing useKCardGrid for the new card grid component - one of the motivations for it was too flexibility and simpler API. So we could see how that works and then have some more experience to consider for use with other emerging components.

@MisRob
Copy link
Member

MisRob commented Sep 30, 2024

Based on the feedback from the team, I'm preparing useKCardGrid for the new card grid component - one of the motivations for it was too flexibility and simpler API. So we could see how that works and then have some more experience to consider for use with other emerging components.

This didn't go that well as I expected. For more reasons, to the best of my knowledge at this point (discussed internally), I'd rather avoid using composable for initial configuration of a component.

So I think I would lean towards one of the options in @BabyElias's original proposal and @AlexVelezLl 's suggestion:

<KTable
  :defaultSort="{ headerId: 'fullName', sort: 'asc' }"
/>

@MisRob MisRob mentioned this issue Oct 4, 2024
5 tasks
@MisRob
Copy link
Member

MisRob commented Oct 4, 2024

It seems we arrived at agreement here.

I didn't want to overwrite the proposal issue so all the discussion doesn't lose valuable context, so I've opened a new final issue out of this proposal here #794, and specified the feature in more detail on that opportunity.

Thanks @BabyElias and @AlexVelezLl :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants