Persist sorting in Browse models #42537
Conversation
|
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Graphite Automations"Notify author when CI fails" took an action on this PR • (05/10/24)1 teammate was notified to this PR based on Raphael Krut-Landau's automation. "Don't backport" took an action on this PR • (05/10/24)1 label was added and 1 reviewer was added to this PR based on Raphael Krut-Landau's automation. |
| :visibility :authenticated | ||
| :type :string | ||
| :default "asc") | ||
|
|
There was a problem hiding this comment.
These are so closely related I thought about storing them together as a JSONified string, or as a string like "+name" or "-name", but this flat, explicit approach seems simplest
There was a problem hiding this comment.
We often will used a closed set of valid choices here. No worries not to, but need to make sure that the FE can handle invalid input.
There was a problem hiding this comment.
If the sort column is invalid - neither "asc" or "desc" - then the FE defaults to ascending
There was a problem hiding this comment.
and also the column to sort on? (no need to respond, just making sure we are defensive)
There was a problem hiding this comment.
Ah yes, to correct my earlier statement: if the sort direction in the API is invalid (or absent) we default to asc.
If the sort column in the API is invalid (or absent), we default to collection.
|
rafpaf
force-pushed
the
browse/preserve-sort
branch
from
f15288b
to
ef10d41
Compare
There was a problem hiding this comment.
- I think persisting sort direction is not a priority for this feature, and introduces unnecessary complexity, for a tiny benefit to users that they will not likely notice.
- this likely adds complexity to improving performance, so I'd prefer to defer this until that is sorted
This branch persists sorting of Browse models table in the API and adds an e2e test
Closes #42952