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

Move DataTable header creation to before updating sort order #3924

Merged
merged 2 commits into from
Nov 13, 2023

Conversation

thyeggman
Copy link
Contributor

@thyeggman thyeggman commented Nov 8, 2023

This PR fixes a bug where sortRows may be called before headers is initialized. sortRows filters headers on this line. I just moved the code block up to avoid this issue.

Changelog

New

NA

Changed

  • Moved header initialization before applying sort order and direction

Removed

NA

Rollout strategy

  • Patch release
  • Minor release
  • Major release; if selected, include a written rollout or migration plan

Testing & Reviewing

Tried a few different iterations of unit tests but I couldn't get one that definitively proved the issue. However, in my own usage of the component, I found that it seems to be related to state change in some way. This is the error that I was seeing:

console error emitted by DataTable

Merge checklist

Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.

Copy link

changeset-bot bot commented Nov 8, 2023

🦋 Changeset detected

Latest commit: ed6f176

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

This PR includes changesets to release 1 package
Name Type
@primer/react 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

Copy link
Contributor

github-actions bot commented Nov 8, 2023

size-limit report 📦

Path Size
dist/browser.esm.js 104.03 KB (0%)
dist/browser.umd.js 104.56 KB (0%)

@github-actions github-actions bot temporarily deployed to storybook-preview-3924 November 8, 2023 22:20 Inactive
@thyeggman thyeggman marked this pull request as ready for review November 8, 2023 22:40
@thyeggman thyeggman requested review from a team and joshblack November 8, 2023 22:40
@thyeggman thyeggman changed the title Move header creation before sort order determination Move DataTable header creation before updating sort order Nov 8, 2023
@thyeggman thyeggman changed the title Move DataTable header creation before updating sort order Move DataTable header creation to before updating sort order Nov 8, 2023
Copy link
Member

@joshblack joshblack left a comment

Choose a reason for hiding this comment

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

🔥 Thanks for the fix!

@joshblack joshblack added this pull request to the merge queue Nov 13, 2023
Merged via the queue into main with commit e512c04 Nov 13, 2023
29 checks passed
@joshblack joshblack deleted the thyeggman/fix-headers-initialization branch November 13, 2023 19:38
@primer primer bot mentioned this pull request Nov 13, 2023
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.

2 participants