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

Table: onClick header and sort stories #233

Merged

Conversation

etienneburdet
Copy link
Contributor

Summary

The goal for this PR is to add a sort system, by making the header clickable and adding a sort direction indicator

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

Changes

I simply added a on:click and an icon in the header. Same as for pagination, we way want to deal with api calls so it doesn't really make sense to implement sort function. The UI is then just a controlled/stateless interface.

Sorties kinda exemplify what the result could be (sort function of the "api" is stupid on only really works on numbers).

Changelog

Table now have a sort mechanism.

Open discussion

The current implementation lacks definitive icons. But maybe we could accept any icon as prop? This opens a lot of questions however (security with SVG, format compitibility etc.), but might not be that hard.

Review checklist

  • Description is complete
  • Commits respect the Conventional Commits Specification
  • 2 reviewers (1 if trivial)
  • Tests coverage has improved
  • Code is ready for a release on NPM

Base automatically changed from feature/add-number-per-pages to main May 21, 2024 07:05
@etienneburdet etienneburdet force-pushed the feature/sc-48046/sdk-table-implement-async-sort-functions branch 2 times, most recently from 59031fd to d5d90b1 Compare May 22, 2024 15:12
@etienneburdet etienneburdet force-pushed the feature/sc-48046/sdk-table-implement-async-sort-functions branch from d5d90b1 to d530319 Compare May 23, 2024 07:57
@etienneburdet etienneburdet changed the title Feature/sc-48046/sdk-table-implement-async-sort-functions Table: onClick header and sort stories May 23, 2024
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.

Personally, I can see UI and UX improvements, but I won't ask to do it in this PR.
Because, at some point, the design team will ask to change it based on a design (hopefully).

So I focused my review only on the technical level

@KevinFabre-ods
Copy link
Contributor

@etienneburdet etienneburdet force-pushed the feature/sc-48046/sdk-table-implement-async-sort-functions branch from 99672a8 to 346cca4 Compare May 24, 2024 07:56
@etienneburdet
Copy link
Contributor Author

etienneburdet commented May 24, 2024

I implemented the design, seems to work easily enough 👌 I just renamed Header.svelte into Headers.svelte, because at some point I contemplated a HeaderCell.svelte component. It's not necessary yet, but might happen and technically it's a row of headers so I kept the plural.

Also CI complains about a missing nx dep. I don't why, build-ci runs fine on my machine. Updating nx seems to do the trick 🤷

@etienneburdet etienneburdet force-pushed the feature/sc-48046/sdk-table-implement-async-sort-functions branch from 346cca4 to f6f956b Compare May 24, 2024 08:01
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.

Looks good !

Just a comment about aria label for the sort button

Also, what your opinion about the answer I gave you for this discussion #233 (comment) ?

@KevinFabre-ods KevinFabre-ods merged commit 8addb80 into main May 27, 2024
8 of 9 checks passed
@KevinFabre-ods KevinFabre-ods deleted the feature/sc-48046/sdk-table-implement-async-sort-functions branch May 27, 2024 10:12
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.

None yet

3 participants