-
Notifications
You must be signed in to change notification settings - Fork 245
feat(query-bar): allow users to set the default sort to be recent first COMPASS-6706 #6663
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
Merged
Merged
Changes from all commits
Commits
Show all changes
24 commits
Select commit
Hold shift + click to select a range
f3174bc
changes
djechlin-mongodb 1d13dbd
changes
djechlin-mongodb 987e78d
Merge branch 'main' into COMPASS-6706
djechlin-mongodb 59e0b25
remove stray comments
djechlin-mongodb b4a3945
copy
djechlin-mongodb 7a56074
changes
djechlin-mongodb b2d9240
changes
djechlin-mongodb af1f596
changes
djechlin-mongodb 879cca9
changes
djechlin-mongodb 82207d6
c
djechlin-mongodb 650a774
this works still
djechlin-mongodb 4e4dd30
changes
djechlin-mongodb 72f0697
c
djechlin-mongodb ddcab6f
c
djechlin-mongodb 60668d2
changes
djechlin-mongodb b7d6a8c
c
djechlin-mongodb 0489bcc
crud test
djechlin-mongodb df8dc6c
leafygreen
djechlin-mongodb c1ed3d9
longer description
djechlin-mongodb d22bea9
remove selectable values code
djechlin-mongodb db27b01
remove a selectable values ref
djechlin-mongodb 7114687
value : string not value: string | undefined
djechlin-mongodb ebbfca0
unit test fix in settings
djechlin-mongodb 69daf52
fix unused var
djechlin-mongodb File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
This changes the behavior only for CRUD, but query bar is also used in the schema tab, I think we might want to move this logic to query bar itself so that it consistently applies every time no matter where current query from query bar is accessed
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.
It's debatable. I'd actually lean toward putting it in the "crud" layer not the "query bar" layer.
Firstly the product question -- if I understand correctly, the schema tab basically ignores
sortand we should actually get rid of that field because it's not helping users at all. If sort were somehow used, I would say no don't use defaults -- for schema users, it's very important they transparently see the inputs to the validation, so silently joining in a default becomes un-helpful to them, whereas it's a non-misleading nice-to-have in document list.From there, I actually would view this as a CRUD/document-list feature not a query bar feature. Users are basically complaining the docs are in the wrong order, they're not complaining that anything is wrong with their queries. In particular, the feature request is to not change recent queries or saved queries at all -- users are trying to not think about the sort being applied for them, they want the docs to just appear in the right order. So that suggests a document-list feature.
If you move it to query-bar you get a bit of complexity as we now have to manage the double concept of "what the user typed" and "what the user typed after defaults are applied." CRUD would then execute the latter. Recent/saved queries only care about user input; if you believe my above argument then schema only cares about user input; doc list cares about the defaults. To me it just looks like this is something doc list wants to do, and we shouldn't assume it readily generalizes to other uses or that we get any leverage out of moving it to query bar.
LMK what you think. If we do want it used in schema then yeah it needs to be more general. Can you think of any other reasons query bar or its other users would need to care about the defaults -- maybe logging or something gets affected, noting that we specifically don't want the sorts showing up in recent/favorites? Any other value to putting it in the more general location of query bar I'm missing? At this point it just looks a little lower complexity/less stateful concepts to manage to put it in CRUD.
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.
We already use maxTimeMS from preferences in query bar to validate user input and in other cases provide default substitutions from query bar defined constants when user input is empty when returning a parsed query, so I don't think it's that far off from query bar responsibilities. It is also true though that maxTimeMS itself while used for validation, is not returned as a default value so I agree that at the current moment we can go both ways about it. I have my preference, but I think your arguments are reasonable too 🙂
I want to touch on this a bit because I think it's an important part of the state management story. I think there is no extra complexity involved here compared to running this logic elsewhere, just actual states (query bar user input, current preferences values, etc) and a derived state of a parsed query that takes multiple of those sources into account. So if it's only technical complexity that stops you from doing this, I don't think it should. But as I mentioned, your other points make sense, so feel free to resolve this if it still makes more sense to you to keep it in CRUD only.
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.
Chatted offline a bit -- confirmed schema tab actually ignores the sort part of the query. From here I think that tilts the argument to the CRUD component because we know the other possible user, schema tab, will actually ignore this data if we send it (and Sergey signed off on my reasoning).