-
Notifications
You must be signed in to change notification settings - Fork 193
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(query-bar): allow users to set the default sort to be recent first COMPASS-6706 #6663
base: main
Are you sure you want to change the base?
Conversation
/** | ||
* Set the default sort. | ||
*/ | ||
showRecentDocumentsFirst: { |
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.
Not sure how in progress this PR is, but just in case, this doesn't seem to match the ticket. Latest design show a dropdown with three choices, not a boolean option
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.
Yep it's the dropdown now
@@ -199,6 +208,8 @@ type PreferenceDefinition<K extends keyof AllPreferences> = { | |||
? boolean | |||
: false | |||
: boolean; | |||
|
|||
selectableValues?: ReadonlyArray<{ value: string; label: string }>; |
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 don't think you need a separate prop to keep values around, this is also disconnected from the actual type which can end up in issues. You already defined them in the schema validator and they can be extracted from validator if needed (something your get settings method can use):
var a = z.enum(['foo', 'bar']);
a.options // ['foo', 'bar']
I like the idea of storing option descriptions here, we already do this for property descriptions so it fits near. I think we can make this type stricter to make sure you're only providing labels for existing expected values:
optionDescription?: AllPreferences[K] extends string ? { [k in AllPreferences[K]]: string } : never;
(maybe even integrate it into exiting description
field)
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.
- options is up to the description now
- I'm hardcoding the dropdown that it's for
defaultSortOrder
and pulling in the array directly. I guess you can unpack it from the validator. This feels a little complicated in a way, like are you supposed to be having the validator pulled into the settings? And the type math is hard to pass the validator around. And there wasn't a lot gained from slightly less direct data access.
I'm a little interested in fully data-driving the setting but there seems to be something genuinely tricky about having these things compile-time forced to be equal:
- the dropdown options
- the legal options via the union type
- the validator options
I'm also new-ish to zod and this level of type validation so maybe it's easier than I think? Let me know if I should take another look at getting the types through the validator.
Description
Checklist
Motivation and Context
Open Questions
Dependents
Types of changes