-
Notifications
You must be signed in to change notification settings - Fork 3
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
feat: sorted, sortable, styled stories #222
Conversation
There was a problem hiding this 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.
Yeah there are a few assumptions that are not obvious right now :
Nope indeed, all of that remain to be done!
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).
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.
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.
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
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 |
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.