-
Notifications
You must be signed in to change notification settings - Fork 390
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
feat: clarify the order by messageId
in v4 and support order by message
#1515
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@@ -37,7 +37,7 @@ export type CatalogFormatOptions = { | |||
disableSelectWarning?: boolean | |||
} | |||
|
|||
export type OrderBy = "messageId" | "origin" | |||
export type OrderBy = "messageId" | "message" | "origin" |
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 thought about renaming or adding linguiId
as an option, but feel like it may be misleading itself, as we do use custom message id as messageId if exists. And support purely order by linguiId
seems not that helpful at the moment. So only adding the message
here.
size-limit report 📦
|
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## next #1515 +/- ##
==========================================
+ Coverage 74.04% 74.19% +0.14%
==========================================
Files 67 67
Lines 1865 1868 +3
Branches 491 494 +3
==========================================
+ Hits 1381 1386 +5
+ Misses 382 380 -2
Partials 102 102
... and 2 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
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.
@taozhou-glean thank you!
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.
Thanks for contribution. I also thinks that sort by message should be a default one. Because this would be less breaking for most supported usecase.
For those who uses explicit ids there always an option to enable "messageId" sort manually
I would +1 on that, can do that in a follow up if @andrii-bodnar also agrees on this. |
@taozhou-glean sounds good to me |
@andrii-bodnar cool, will change the default |
Description
SInce the hash id support is in, the
messageId
now becomes thelinguiId
when custom id is absent. This is a change of behavior compared to v3, so its worth clarifying in the doc.And to preserve the v3 behavior as much as we can, I am adding a new orderBy (
message
), so when no custom id is used, the order bymessage
should match whatmessageId
is expected in v3.Types of changes
Fixes # (issue) #1514
Checklist