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

feat: sorted, sortable, styled stories #222

Conversation

etienneburdet
Copy link
Contributor

@etienneburdet etienneburdet commented Jan 31, 2024

Summary

The goal for this PR is to to propose a props definition / API for the Table component. Most likely serve as the "main" PR for the table implem.

(Internal for Opendatasoft only) Associated Shortcut ticket: sc-46238.

Changes

Changes to review are mainly in types.ts, which is what we expose.

The Table.svelte is a base implementation just to check that we don't need extra props info etc. to implement the base details. Implem itself is pretty dirty as of now.

Table.stories.tsx is here to have a variety of test cases that our implem should fit. Potentially the place to add tests too.

Copy link
Contributor

@KevinFabre-ods KevinFabre-ods left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for you exploration. I'm mostly in phase with the API. Using an array of columns to define how the data is displayed IMO is the right move.

I have outlined several points for discussion:

  • We do not see how to add translations for table elements with interactions, such as 'previous' and 'next' buttons for pagination, as well as for page navigation buttons (e.g., 'go to page n'). The buttons for triggering sorting also need consideration (I don’t think we need a sort.label; we can rely on just column.title to construct the label).
  • There is no mention of Right-to-Left (RTL) and Left-to-Right (LTR) support. Should we include a property for that?
  • The definition of types is a good thing. I wonder if it would be simpler to have a mandatory 'type' key, and an optionnal 'format' key that allows displaying custom formats if the default type formatting is not suitable. IMO, this formatting option should not be present if we handle many types. Do we agree that if I want to display a date in year and another in year-month-day, I don't need to use the format function, right?
  • What was the idea behind different types like long-text and short-text, as well as integer and float?
  • Is the 'width' property mandatory for a 'fixed' column? At first glance, it does not seem intuitive to define two properties for a single behavior.
  • If we add a footer row at the bottom of the table to display a sum, average, or other, do we add a property to the Column type?
  • There is no reference to customizing the style of a cell. How can I specify that a cell has color 'a' if its value is 'x'? IMO, the format function shouldn't be the awser? Otherwise, what's the point of defining types?
  • There is no reference to adding custom classes. We see an 'unstyled' prop, but how do I add a class for a header cell or a cell displaying value?
  • Okay for having an object property to control the visibility of the pagination component. However, I have doubts about the names of the pagination keys for future versions. We can easily predict that there will be a button to choose the number of rows to display. From then on, 'total' should no longer refer to the number of pages but to the number of records, so the pagination component can calculate the number of pages on the fly (number of items / number of items to display).
  • IMO the 'onSort' key cannot remain in the column object. It should be moved to the root of the API so that this prop only needs to be constructed once by the context that render the table component. Instead, in the column object, I would add a 'sortable' property (boolean).
  • IMO If we want to enable multi-sorting, the 'defaultSortKey' property should be an array of strings?

In the story, there was a reference to rules for managing row grouping, but we will not propose this option. It does not follow good data visualization practices for the tables we are designing.

@etienneburdet
Copy link
Contributor Author

Yeah there are a few assumptions that are not obvious right now : 

  • For styling, I think we can rely on a wrapper class and just style th, td, tr… etc. I think we can select everything easily this way and it's pretty generic. Hence the unstyled props to get rid of all default styling on those and avoid overwriting them.
  • For the format function, my idea is to have either a custom callback or a string ('long-text' etc.). If it's a valid string, then we have premade formatting callbacks. We could also just expose some common formatting callbacks (number, long-text etc.), which I didn't do yet.
We do not see how to add translations for table elements with interactions, such as 'previous' and 'next' buttons for pagination, as well as for page navigation buttons (e.g., 'go to page n'). The buttons for triggering sorting also need consideration (I don’t think we need a sort.label; we can rely on just column.title to construct the label).
There is no mention of Right-to-Left (RTL) and Left-to-Right (LTR) support. Should we include a property for that?

Nope indeed, all of that remain to be done!

Is the 'width' property mandatory for a 'fixed' column? At first glance, it does not seem intuitive to define two properties for a single behavior.

A bit of the same idea as with formatting: it can be a number (then it's fixed) or something like 'auto' and then the component try to "fit" (whatever that means).

If we add a footer row at the bottom of the table to display a sum, average, or other, do we add a property to the Column type?
Okay for having an object property to control the visibility of the pagination component. However, I have doubts about the names of the pagination keys for future versions. We can easily predict that there will be a button to choose the number of rows to display. From then on, 'total' should no longer refer to the number of pages but to the number of records, so the pagination component can calculate the number of pages on the fly (number of items / number of items to display).

I kinda made the assumption that the component doesn't do any computation on pagination, you really pass props related to display. It leaves the division to the user, but has one props and allows for marginal cases with stuff like different number of records per pages (if the callback is a bit weird). But the main idea really is "we just display what you pass, we don't do any computation". It can be challenged though.

IMO the 'onSort' key cannot remain in the column object. It should be moved to the root of the API so that this prop only needs to be constructed once by the context that render the table component. Instead, in the column object, I would add a 'sortable' property (boolean).

You could have different ways to sort different columns. In the story I sort one alphabetically, but the other by text length. But we could have default based on column type/format.

IMO If we want to enable multi-sorting, the 'defaultSortKey' property should be an array of strings?

I don't think we want multi-sorting no? I don't see how we handle order with a good UX.

stories are not fully working yet
@KevinFabre-ods
Copy link
Contributor

Thanks for the exploration. We'll be adding these properties as we go along and depending on the feedback we'll receive.

First PR is available here: #223

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.

2 participants