-
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
Feature/sc-46253/sdk-table-pagination #229
Conversation
0d91fd1
to
90e6122
Compare
@cybervinvin @KevinFabre-ods Don't mind the style/css too much in this one, I proposed something different in #230 . |
e9b999e
to
49dee4c
Compare
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.
Left some comments about the table behavior with the pagination.
For the eslint changes, what did you encounter to make theses edits ?
Other than that, pagination works well. Maybe we want the current page to react to a change in the API (when initial
changes)
packages/visualizations-react/stories/Table/Pagination.stories.tsx
Outdated
Show resolved
Hide resolved
/* Preserves paginations controls positioning | ||
min heigh of table + controls = max-height of row * (number of rows + 1 for headers) | ||
*/ | ||
$: minHeight = pages ? `${MAX_ROW_HEIGHT * (pages.recordsPerPage + 1)}px` : 'none'; |
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.
Did we check with the design for this behavior ?
Can be weird if we display 20 items per page with only one item for the last page. We get a lot of white space
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.
No we still have to submit that to Meg, but it's really annoying to have the pagination moving between pages too.
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.
We asked the question https://app.shortcut.com/opendatasoft/story/49140/design-how-the-table-and-pagination-react-when-number-of-items-changes-between-two-pages. Waiting for an answer...
In the meantime, I'm in favor of adding the less code possible. I prefer to add a function or behavior later rather than delete one.
packages/visualizations/src/components/Table/Pagination/Button.svelte
Outdated
Show resolved
Hide resolved
packages/visualizations/src/components/Table/Pagination/Pagination.svelte
Outdated
Show resolved
Hide resolved
packages/visualizations/src/components/Table/Pagination/Pagination.svelte
Outdated
Show resolved
Hide resolved
packages/visualizations/src/components/Table/Pagination/Pagination.svelte
Outdated
Show resolved
Hide resolved
<Body {records} {columns} /> | ||
{/if} | ||
</table> | ||
<div class="table-controls" style="--min-height: {minHeight}"> |
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.
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.
It's for holding the pagination in the same place relative to the table. I think it's fixed in #230
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.
Some comments about how pagination numbers are computed
packages/visualizations/src/components/Table/Pagination/Pagination.svelte
Outdated
Show resolved
Hide resolved
packages/visualizations/src/components/Table/Pagination/Pagination.svelte
Outdated
Show resolved
Hide resolved
Coverage after merging feature/sc-46253/sdk-table-pagination into main
Coverage Report
|
Sorry the eslint thing got a bit bigger than I thought and ideally should be for this PR… but well, at least it's done. |
Coverage after merging feature/sc-46253/sdk-table-pagination into main
Coverage Report
|
Soooo… I ended up removing our customization of Now just removing the eslint for curly new line works: what we did before is still good with prettier (because of the Prettier still won't let us do something like:
|
409183d
to
ab2199f
Compare
Coverage after merging feature/sc-46253/sdk-table-pagination into main
Coverage Report
|
Coverage after merging feature/sc-46253/sdk-table-pagination into main
Coverage Report
|
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.
After review, I don't have much to say.
There are several things that I find strange but since I don't know the best practices in the SDK, I prefer not to start discussions here.
After using it in the studio, the component API seems OK, so LGTM 👍🏼
$: if (totalPages === 2) { | ||
pages = [1, 2]; | ||
} else if (totalPages === 1) { | ||
pages = [1]; | ||
} else if (current - 1 < 1) { | ||
pages = [current, current + 1, current + 2]; | ||
} else if (current + 1 > totalPages) { | ||
pages = [current - 2, current - 1, current]; | ||
} else { | ||
pages = [current - 1, current, current + 1]; | ||
} |
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.
It's hard to read, IMO we can improve it:
If totalPages is equal to 3 are less: pages = Array.from({length: totalPages}, (_, i) => i + 1)
if current === 1: pages = [current, current + 1 , current + 2];
if current === totalPages: pages = [current - 2, current - 1, current];
All other cases: pages = [current - 1, current, current + 1];
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.
Ha yes, we can change that computation now that we've changed the indexes to start at 1.
|
||
const paginatedOptions = { | ||
...options, | ||
pages: { |
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.
pages: { | |
pagination: { |
@@ -10,7 +11,7 @@ | |||
</script> | |||
|
|||
<!-- To display a format value, rawValue must be different from undefined or null --> | |||
<td class={`table-data--${dataFormat}`}> | |||
<td class={`table-data--${dataFormat}`} style="max-height: {MAX_ROW_HEIGHT}px"> |
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.
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.
Pagination snapshots in Chromatic don't have the pagination element => due to a missing rename (pages => pagination)
Other than that, I think we can improve the readability of the code to computed the page buttons.
Also waiting for your input about the meeting you had with Meg for the pagination behavior.
Finally don't hesitate to reply to my comments when you did or didn't change the code. With a force push commit, comments are detached from the code (sometimes it takes some time to find again the part of the code related to the comment)
2f96758
to
d918458
Compare
Coverage after merging feature/sc-46253/sdk-table-pagination into main
Coverage Report
|
Coverage after merging feature/sc-46253/sdk-table-pagination into main
Coverage Report
|
e4eee8e
to
7cb5135
Compare
Coverage after merging feature/sc-46253/sdk-table-pagination into main
Coverage Report
|
Coverage after merging feature/sc-46253/sdk-table-pagination into main
Coverage Report
|
a2c0a8f
to
9d92b0d
Compare
Coverage after merging feature/sc-46253/sdk-table-pagination into main
Coverage Report
|
1 similar comment
Coverage after merging feature/sc-46253/sdk-table-pagination into main
Coverage Report
|
Coverage after merging feature/sc-46253/sdk-table-pagination into main
Coverage Report
|
1 similar comment
Coverage after merging feature/sc-46253/sdk-table-pagination into main
Coverage Report
|
fc103bc
to
d3899da
Compare
Coverage after merging feature/sc-46253/sdk-table-pagination into main
Coverage Report
|
1 similar comment
Coverage after merging feature/sc-46253/sdk-table-pagination into main
Coverage Report
|
Coverage after merging feature/sc-46253/sdk-table-pagination into main
Coverage Report
|
1 similar comment
Coverage after merging feature/sc-46253/sdk-table-pagination into main
Coverage Report
|
Coverage after merging feature/sc-46253/sdk-table-pagination into main
Coverage Report
|
1 similar comment
Coverage after merging feature/sc-46253/sdk-table-pagination into main
Coverage Report
|
ae7a29e
to
8ca5f6d
Compare
Coverage after merging feature/sc-46253/sdk-table-pagination into main
Coverage Report
|
1 similar comment
Coverage after merging feature/sc-46253/sdk-table-pagination into main
Coverage Report
|
8ca5f6d
to
26a0713
Compare
Coverage after merging feature/sc-46253/sdk-table-pagination into main
Coverage Report
|
1 similar comment
Coverage after merging feature/sc-46253/sdk-table-pagination into main
Coverage Report
|
We'll change that later anyways, but fixes chromatic diff.
26a0713
to
14a3278
Compare
Summary
The goal for this PR is to pagination to the table component
(Internal for Opendatasoft only) Associated Shortcut ticket: sc-46253.
Changes
I added a pagination block with options to be styled. It should be disabled when hitting the lower bounds, show ellipsis when there are too much pages and accept an async callbacks.
Stories should reflect all those changes.
I left out the "number per pages". Although the component is here, the way we handle the callback isn't that easy (or actually doesn't really fit with the way we handle the change page callback).
Changelog
"Tables now support paginitaion"
Open discussion
So I stick mostly with our current way of styling, but I think we leverage the new implementation better, by using a "className" props that we can now pass safely. It would avoid wrapping and probably reduce a bit of internal wrapping too.
We can't leverage
ods-visualizations-table
though (as I initialy hope we could), since it's only present in the react implem for now.Review checklist