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

Rerender the Table when rows change #1306

Merged
merged 4 commits into from
Dec 7, 2021
Merged

Rerender the Table when rows change #1306

merged 4 commits into from
Dec 7, 2021

Conversation

robinmetral
Copy link
Contributor

@robinmetral robinmetral commented Dec 7, 2021

Follows up on #1292

Purpose

#1292 introduced a bug where if the Table's rows prop got an update, the table itself wouldn't get updated.

This is an issue when rows state is managed in userland, for example when table filtering is implemented.

Approach and changes

  • Set rows to stete on mount
  • Update rows on update if necessary
  • Preserve sort on update

Definition of done

  • Development completed
  • Reviewers assigned
  • Unit and integration tests
  • Meets minimum browser support
  • Meets accessibility requirements

@robinmetral robinmetral requested a review from a team as a code owner December 7, 2021 15:09
@robinmetral robinmetral requested review from amelako and removed request for a team December 7, 2021 15:09
@changeset-bot
Copy link

changeset-bot bot commented Dec 7, 2021

🦋 Changeset detected

Latest commit: 822e72d

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@sumup/circuit-ui Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@vercel
Copy link

vercel bot commented Dec 7, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/sumup/oss-circuit-ui/AxLRycSKughTy6YUz9jrX4acugMf
✅ Preview: https://oss-circuit-ui-git-fix-table-update-sumup.vercel.app

@sumup-oss sumup-oss deleted a comment from sumup-clark bot Dec 7, 2021
@codecov
Copy link

codecov bot commented Dec 7, 2021

Codecov Report

Merging #1306 (822e72d) into main (d76415b) will decrease coverage by 0.12%.
The diff coverage is 37.50%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1306      +/-   ##
==========================================
- Coverage   92.34%   92.22%   -0.13%     
==========================================
  Files         186      186              
  Lines        3671     3677       +6     
  Branches     1147     1148       +1     
==========================================
+ Hits         3390     3391       +1     
- Misses        264      269       +5     
  Partials       17       17              
Impacted Files Coverage Δ
packages/circuit-ui/components/Table/Table.tsx 88.50% <37.50%> (-5.33%) ⬇️
...mponents/NotificationBanner/NotificationBanner.tsx 81.96% <0.00%> (-1.64%) ⬇️
...omponents/Table/components/TableBody/TableBody.tsx 100.00% <0.00%> (+9.09%) ⬆️

@robinmetral robinmetral changed the title Update the Table when the rows prop gets nested updates Rerender the Table when rows change Dec 7, 2021
@robinmetral robinmetral added the 🚢 ready to merge Automatically merge the PR once all requirements are met label Dec 7, 2021
@kodiakhq kodiakhq bot merged commit 062aaa1 into main Dec 7, 2021
@kodiakhq kodiakhq bot deleted the fix-table-update branch December 7, 2021 16:16
@github-actions github-actions bot mentioned this pull request Dec 7, 2021
@@ -207,7 +207,7 @@ type TableState = {
class Table extends Component<TableProps, TableState> {
state: TableState = {
sortedRow: undefined,
sortedRows: undefined,
rows: undefined,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be initialized from props, otherwise, the rows aren't shown on the initial render with breaks SSR and can cause other errors.

.changeset/cuddly-brooms-sip.md Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🗂 circuit-ui 🚢 ready to merge Automatically merge the PR once all requirements are met
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants