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

Feature/add-number-per-pages #232

Merged
merged 30 commits into from
May 21, 2024
Merged

Conversation

etienneburdet
Copy link
Contributor

@etienneburdet etienneburdet commented Apr 5, 2024

Summary

The goal for this PR is to add page size options to pagination.

(Internal for Opendatasoft only) Associated Shortcut ticket: sc-46253. (same as base pr

Changes

The biggest change is that I removed all kind of internal state for the current page or the current number per page. This proves impractical when you add number per pages, leading to having to track the current page in the app on top of the SDK component itself. So now the current page and the current number per page are fully controlled and determined by a single prop.

The select is a native select too. We currently don't support children/slots, so it's possible to use a platform select or something like that. But a native select should be fine for the use.

Options have both a label and a value because we may want to add extra wording (e.g. "5 / page") or other labelling ("small", "large" etc.)

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

@etienneburdet etienneburdet changed the base branch from chore/simplify-table-styling to feature/sc-46253/sdk-table-pagination April 18, 2024 13:57
Copy link

Coverage after merging feature/add-number-per-pages into feature/sc-46253/sdk-table-pagination

94.64%

Coverage Report
FileBranchesFuncsLinesUncovered Lines
src
   index.ts100%100%100%
src/client
   error.ts100%100%100%
   index.ts74.03%100%95.31%102–103, 124, 13, 146, 148, 148–149, 15, 15, 151, 162, 169, 169, 17, 17, 171, 176, 179, 182, 184, 52, 82
   types.ts100%100%100%
src/odsql
   clauses.ts71.43%80%90.91%14, 32, 42
   index.ts83.72%95.74%94.19%111, 146, 25, 28, 56–57, 57, 57–58, 68, 78–79

1 similar comment
Copy link

Coverage after merging feature/add-number-per-pages into feature/sc-46253/sdk-table-pagination

94.64%

Coverage Report
FileBranchesFuncsLinesUncovered Lines
src
   index.ts100%100%100%
src/client
   error.ts100%100%100%
   index.ts74.03%100%95.31%102–103, 124, 13, 146, 148, 148–149, 15, 15, 151, 162, 169, 169, 17, 17, 171, 176, 179, 182, 184, 52, 82
   types.ts100%100%100%
src/odsql
   clauses.ts71.43%80%90.91%14, 32, 42
   index.ts83.72%95.74%94.19%111, 146, 25, 28, 56–57, 57, 57–58, 68, 78–79

Copy link

Coverage after merging feature/add-number-per-pages into feature/sc-46253/sdk-table-pagination

94.64%

Coverage Report
FileBranchesFuncsLinesUncovered Lines
src
   index.ts100%100%100%
src/client
   error.ts100%100%100%
   index.ts74.03%100%95.31%102–103, 124, 13, 146, 148, 148–149, 15, 15, 151, 162, 169, 169, 17, 17, 171, 176, 179, 182, 184, 52, 82
   types.ts100%100%100%
src/odsql
   clauses.ts71.43%80%90.91%14, 32, 42
   index.ts83.72%95.74%94.19%111, 146, 25, 28, 56–57, 57, 57–58, 68, 78–79

1 similar comment
Copy link

Coverage after merging feature/add-number-per-pages into feature/sc-46253/sdk-table-pagination

94.64%

Coverage Report
FileBranchesFuncsLinesUncovered Lines
src
   index.ts100%100%100%
src/client
   error.ts100%100%100%
   index.ts74.03%100%95.31%102–103, 124, 13, 146, 148, 148–149, 15, 15, 151, 162, 169, 169, 17, 17, 171, 176, 179, 182, 184, 52, 82
   types.ts100%100%100%
src/odsql
   clauses.ts71.43%80%90.91%14, 32, 42
   index.ts83.72%95.74%94.19%111, 146, 25, 28, 56–57, 57, 57–58, 68, 78–79

@etienneburdet etienneburdet marked this pull request as ready for review April 19, 2024 12:49
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.

At first sight, everything looks good.

Expect a diff in a Chromatic snapshot that, IMO, should not be there.

I'll let @cybervinvin do the functional review as he implementing the table in the platform

packages/api-client/package-lock.json Outdated Show resolved Hide resolved
@etienneburdet etienneburdet force-pushed the feature/sc-46253/sdk-table-pagination branch 2 times, most recently from 26a0713 to 14a3278 Compare April 30, 2024 13:10
Base automatically changed from feature/sc-46253/sdk-table-pagination to main April 30, 2024 13:19
Why didn't that pop in the previous PR?
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.

Works fine.

Maybe we could take some times to discuss how each other see if the table should prevent fail behaviors from its events

package-lock.json Outdated Show resolved Hide resolved
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.

Let's merge!

@KevinFabre-ods KevinFabre-ods merged commit 4ea6838 into main May 21, 2024
9 checks passed
@KevinFabre-ods KevinFabre-ods deleted the feature/add-number-per-pages branch May 21, 2024 07:05
@KevinFabre-ods KevinFabre-ods mentioned this pull request May 21, 2024
5 tasks
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

2 participants