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

Data Table refactor and cleanup #217

Closed
10 tasks done
endigo9740 opened this issue Sep 10, 2022 · 21 comments · Fixed by #520
Closed
10 tasks done

Data Table refactor and cleanup #217

endigo9740 opened this issue Sep 10, 2022 · 21 comments · Fixed by #520
Assignees
Labels
enhancement New feature or request feature request Request a feature or introduce and update to the project. ready to test Ready to be tested for quality assurance.

Comments

@endigo9740
Copy link
Contributor

endigo9740 commented Sep 10, 2022

These updates may include, but are not limited to the following:

  • Create Tailwind Elements stylesheet for native HTML styling
  • Make considerations for "presentational" tables (native or simple component?)
  • Make considerations for fully-featured data tables that include search/sort/selection
  • Make considerations for implementing table features as Svelte Actions
  • Request: allow for hidden column values, which are present on row click
  • Request: multi-row selection
  • Ensure support for Paginator component
  • Request: Improve Typescript typing per Rhys' suggestions
  • Polish and refine
  • Upate documentation
@endigo9740 endigo9740 self-assigned this Sep 10, 2022
@endigo9740 endigo9740 added documentation Improvements or additions to documentation enhancement New feature or request labels Sep 10, 2022
@endigo9740
Copy link
Contributor Author

The use case is something like this - imagine you had a table mapped with four columns:

foo | bar | fizz | buzz

But you wanted to include extra details included in your source data but not visually present it on in the UI. Something like this:

id (hidden) | foo | bar | fizz | buzz

Currently when tapping the row we only present the data visibly present in the data mapping. However, we should consider providing the full set of data for the row from the original source.

@endigo9740
Copy link
Contributor Author

Reference @ryceg proposal for handling data types for data table components:

@endigo9740 endigo9740 changed the title Data Table reactor and cleanup Data Table refactor and cleanup Oct 9, 2022
@endigo9740
Copy link
Contributor Author

Note to self: native HTML tables + action. Perhaps with a Table component just for a simpler interface for generating HTML tables?

@niktek
Copy link
Contributor

niktek commented Oct 13, 2022

Just moving to here:

Issues with column collapsing on individual rows

@niktek
Copy link
Contributor

niktek commented Oct 31, 2022

Just as a note:
There were two exports from DataTableService that are used internally of DataTable.svelte (sortAsc and sortDesc), then there is a mapTableSource that is used in the doc page for data-tables.

On the docs page there is also a CodeBlock that shows an import of import { mapTableSource } from '@brainandbones/skeleton/components/Table/DataTableService'; which I'm pretty sure won't work as the alias @brainandbones/skeleton points to the src/index.ts rather than a directory alias.

What probably needs to happen is that these DataTableService exports need to migrate to the top level src/index.ts file and probably have a bit of a namespace scoping like dtsSortAsc or something like that ?

==
In the meantime, they have been added to the src/index.ts as exports to assist with the monorepo migration

@endigo9740
Copy link
Contributor Author

Adding this for additional context to Nik's note above:

@endigo9740 endigo9740 pinned this issue Nov 3, 2022
@kimjunsung04
Copy link

Feature request: add table pagination

@endigo9740
Copy link
Contributor Author

@endigo9740 endigo9740 unpinned this issue Nov 6, 2022
@endigo9740
Copy link
Contributor Author

@jonas-dongowski
Copy link

Feature request: Add an option to style the currently selected row as it's currently possible to style the row that's being hovered over

@jonas-dongowski
Copy link

Feature request: Add an option to enable multi row selection

@endigo9740 endigo9740 pinned this issue Nov 8, 2022
@endigo9740 endigo9740 added feature request Request a feature or introduce and update to the project. and removed documentation Improvements or additions to documentation labels Nov 9, 2022
@endigo9740
Copy link
Contributor Author

I've updated the original post at the top with desired updates up until this point. Expect updates throughout the week as I implement these changes.

@endigo9740 endigo9740 linked a pull request Nov 9, 2022 that will close this issue
@endigo9740
Copy link
Contributor Author

endigo9740 commented Nov 9, 2022

A new draft PR has been setup to preview progress:

I've managed to make good progress on this today. This includes the follow.

A new Tailwind Element stylesheet and documentation page has been setup for Tables. These provide the default styling values to table components.

Screen Shot 2022-11-09 at 5 49 41 PM

Next, I've created a new simple presentation table component called Table.

  • This uses the data-driven model and is a drop-in replacement for the current documentation tables.
  • Additionally, the component docs detail the utility methods I've exposed to make it easier to format table source data.
  • The basic table also comes with it's own Typescript type to make implementation easier

Screen Shot 2022-11-09 at 5 42 37 PM

Finally, I've stubbed out a documentation page for the power user component called DataTable. This will be a template-driven table that most likely utilizes Actions for features such as sort. Lots of big ideas here, so expect to see more progress over the coming days...

Screen Shot 2022-11-09 at 5 42 49 PM

@endigo9740
Copy link
Contributor Author

This is coming along nicely! I've switched the example to use async data from an HTTP fetch call. But everything here is working as you might expect, minus the janky sorting heh. I'll aim to tackle that tomorrow, along with the following:

  • Move the fetch call to a SvelteKit load function
  • Support for local pagination
  • Support for async features such as search, sort, pagination, etc
  • Implement a11y (most likely as an Action; which will be shared with the standard Table component)

Preview: https://skeleton-gy60mtvgz-skeleton-ui.vercel.app/utilities/data-tables

image

@endigo9740
Copy link
Contributor Author

I have adjusted a few things today, a11y and pagination are now mostly working. However, I feel like the state management is really making things complicated. I think next go I'm going to look to convert this model to a store. This should greatly simplify the means to keep everything in sync and update to date!

Screen Shot 2022-11-11 at 6 06 52 PM

@endigo9740 endigo9740 added the ready to test Ready to be tested for quality assurance. label Nov 12, 2022
@endigo9740
Copy link
Contributor Author

endigo9740 commented Nov 12, 2022

The table has been updated to use writable stores for state. All features (for release) have been implemented. Plus everything is documented.

I've updated the PR from draft to "ready for review". Please begin testing this and supply any feedback here. Let's aim to do this asap so we have time to resolve issues prior to launch please!

Thanks!

@endigo9740
Copy link
Contributor Author

NOTE: nearly all items reported by @ryceg on the PR comments have been resolved. Just awaiting feedback on the two items remaining.

@endigo9740
Copy link
Contributor Author

Noting this - we should implement more style props and customization options for the simple Table component.

@niktek
Copy link
Contributor

niktek commented Nov 14, 2022

missing values in sourceData show as undefined in cell:
If you delete the name prop from one of the rows in the sourceData, it displays undefined. This is kind of on the end dev to sanitise before it gets to the DataTable, but it might be good to output '' instead ?

keyboard selecting of row:
Safari - once you tab into the rows, you can navigate the cells with the right/left/up/down etc. And on the select column with the checkbox input, you have to tab into it again to then be able to select it on or off. It would be great if you could select the row of whatever table cell is currently highlighted.

This could also be done by updating the sample to paste in an on keydown handler ? this would also get rid of the a11y warnings that SK spits out.

search:
not sure when the filtering kicks in - but type a single letter 'b' and oxygen, nitrogen, etc remain

does not show sorting arrows in first display:
On first display (or refresh of page) the sorting arrows on the head columns do not display until the user clicks on them, which limits user discoverability.

dataTableSelect selects on first page of data:

shows on first page of data, sort the table and it incorrectly stickies the choice, click column head again - back to the correct first page with selection, click column head again and now no selection is shown.

done with dataTableSelect(dataTableModel, 'position', [8,2]);

@endigo9740
Copy link
Contributor Author

missing values in sourceData show as undefined in cell: If you delete the name prop from one of the rows in the sourceData, it displays undefined. This is kind of on the end dev to sanitize before it gets to the DataTable, but it might be good to output '' instead?

Wasn't sure if this was in the Table or Data Table as you did not specify. Skeleton is only responsible for this in the Table component since we control what happens to the data. Currently this handles fine due to this condition:

{@html cell ? cell : '-'}

So I'm assuming you mean in the data table, where as you mentioned, users will be responsible for sanitizing their own data.

keyboard selecting of row: Safari - once you tab into the rows, you can navigate the cells with the right/left/up/down etc. And on the select column with the checkbox input, you have to tab into it again to then be able to select it on or off. It would be great if you could select the row of whatever table cell is currently highlighted.

Not sure we can squeeze this in before release. But should make for a great candidate for future updates.

This could also be done by updating the sample to paste in an on keydown handler ? this would also get rid of the a11y warnings that SK spits out.

I realized I forgot to to copy over the on:keypress event handler for the example, so this should now follow what the example is doing.

search: not sure when the filtering kicks in - but type a single letter 'b' and oxygen, nitrogen, etc remain

I've made some modifications to the fuzzy search to help with this. Previously I would do this:

  1. Take the entire row object, ex: {foo: 'bar', fizz: 'buzz'}
  2. Running JSON.stringify() on to convert to a string
  3. Convert the search term and this object string to lower case
  4. Searching for a matching string value

This works, but this means object keys were being searched too, not just the values. So I've now I'm using Object.keys() to grab only the values visible in each cell and searching against them. So previously b would show all results because every row had body: x as a key!

does not show sorting arrows in first display: On first display (or refresh of page) the sorting arrows on the head columns do not display until the user clicks on them, which limits user discoverability.

I agree, but I was running into some bugs with this. May need to revisit post launch. Essentially there's a disconnect between the UI, the action, and the sort feature itself that I need to try and work around.

dataTableSelect selects on first page of data: shows on first page of data, sort the table and it incorrectly stickies the choice, click column head again - back to the correct first page with selection, click column head again and now no selection is shown.

Yeah there was some major bugs between sort/pagination. So I've gone through and did a bit of refactor. It seems to be producing the expected results now though!

@endigo9740
Copy link
Contributor Author

endigo9740 commented Nov 14, 2022

Since we won't be able to fit EVERY request in this release, I've created this ticket to track requested updates:

@endigo9740 endigo9740 unpinned this issue Nov 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request feature request Request a feature or introduce and update to the project. ready to test Ready to be tested for quality assurance.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants