-
Notifications
You must be signed in to change notification settings - Fork 462
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
fix: archived releases table has default sort and defined sort working #9010
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
2 Skipped Deployments
|
No changes to documentation |
⚡️ Editor Performance ReportUpdated Sat, 22 Mar 2025 12:42:42 GMT
Detailed information🏠 Reference resultThe performance result of
🧪 Experiment resultThe performance result of this branch
📚 Glossary
|
📊 Playwright Test ReportThis report contains test results, including videos of failing tests. |
d669f39
to
cdce9a6
Compare
sorting: true, | ||
width: 100, | ||
sortTransform: ({publishedAt, _updatedAt}) => { | ||
// the default sort is always descending, so -Infinity pushes missing values to end |
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.
Very smart!
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.
I think the solution is pretty smart. I'm still unsure how I feel about a hidden column. That being said, the code overall looks good to me. I think I would prefer to have someone else have a look at it too and have some more thoughts, maybe @pedrobonamin since I think you also did quite a bit of work on this table?
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.
I think it's a very clever solution, fixes the issue without introducing too many changes to the codebase.
Description
_updatedAt
orpublishedAt
depending on which is availableWhat to review
Supporting
hidden
columns in theTable
- this is a bit of a hack just to support this particular case (where we want the default sorting to be across multiple columns. An alternative abstraction would be to move a function for default sorting out to theTable
prop surface, but given theTable
is only used within releases in 2 locations, going to those extents doesn't seem appropriate.Testing
No new tests added, as Table isn't currently covered by tests
Notes for release
Fix for incorrect ordering when sorting archived and published releases in the Content Releases tool