feat: notes display criteria #232
Conversation
test/collections.test.js
Outdated
| @@ -65,6 +65,20 @@ describe('payload collections', () => { | |||
| expect(collection.conflictsOf(payload.uuid)).to.eql([manualResults]); | |||
| }); | |||
|
|
|||
| it.skip('setting same element twice should not yeild duplicates', async () => { | |||
There was a problem hiding this comment.
This test doesn't pass, and is a new one created to catch a pre-existing issue. I didn't want to modify this behavior in this PR, so marked it as skip for now.
packages/snjs/lib/protocol/collection/notes_display_criteria.ts
Outdated
Show resolved
Hide resolved
packages/snjs/lib/protocol/collection/notes_display_criteria.ts
Outdated
Show resolved
Hide resolved
| public subFilter?: (element: any) => boolean; | ||
|
|
||
| static CreateCriteria( | ||
| initializer: (criteria: NotesDisplayCriteria) => void |
There was a problem hiding this comment.
With Partial<NotesDisplayCriteria> you can do this:
NotesDisplayCriteria.Create({
includePinned: true
});The object doesn't have to be an instance of NotesDisplayCriteria, it just has to respond identically to its property calls. See how only the last call in this example is invalid: https://www.typescriptlang.org/play?#code/MYGwhgzhAEAqAWBLAdgc2gbwFDWgBwFcAjERYaAMwHsroBeSsECAUwG4d9jTyiwAnetAAu-AuywBfLFgoFkwYYirIRYANYsICFKgAUwpGgBc0AAoClTADw60APgCUmTsBUQqIFgDoQVfYa63tRUjhzSWMIaWnb6GJJhkdHaRnGcIaai4lKJUZopunrYuHz8mWIsORx5MalFnKWmAEQA7ioA5MLQbgC2eIheTVVAA
In the end both approaches look kind of similar on the calling site and I don't hold a strong opinion either way, especially since this pattern also exists in other parts of the app. Just wanted to throw this at you in case you thought going the "pass an object" approach was impossible with TypeScript :)
packages/snjs/lib/protocol/collection/notes_display_criteria.ts
Outdated
Show resolved
Hide resolved
| sortBy?: CollectionSort, | ||
| direction?: SortDirection, | ||
| filter?: (element: SNNote) => boolean | ||
| public setNotesDisplayCriteria( |
There was a problem hiding this comment.
I meant that SNApplication.setDisplayOptions() (the more generic method that covers everything except for notes, a couple of lines above) should be renamed to SNApplication.setDisplayCriteria(), because while the method signature is different the intent is the same.
Yeah you're right, this is a cleaner approach. Will update. |
Oh, the only thing is that display criteria right now is just for notes. Only NotesDisplayCriteria exists. |
No description provided.