-
-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Desktop: Fixes #4411: Count tags based on showCompletedTodos setting #4957
Conversation
@@ -448,7 +448,7 @@ class SidebarComponent extends React.Component<Props, State> { | |||
|
|||
renderTag(tag: any, selected: boolean) { | |||
const anchorRef = this.anchorItemRef('tag', tag.id); | |||
const noteCount = Setting.value('showNoteCounts') ? this.renderNoteCount(tag.note_count) : ''; | |||
const noteCount = Setting.value('showNoteCounts') ? Setting.value('showCompletedTodos') ? this.renderNoteCount(tag.note_count) : this.renderNoteCount(tag.note_count - tag.todo_completed_count) : ''; |
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.
Could you refactor this logic to avoid the nested ternary operators? This is to clarify a bit the logic.
packages/app-cli/tests/models_Tag.ts
Outdated
@@ -123,21 +133,25 @@ describe('models_Tag', function() { | |||
expect(commonTags.length).toBe(0); | |||
|
|||
commonTags = await Tag.commonTagsByNoteIds([note0.id, note1.id, note2.id, note3.id]); | |||
// @ts-ignore |
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.
Why do we need ts-ignore here?
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 can't figure out why I added the ts-ignore, I have removed them.
Ok I have made the performance test on Android, with the following items count:
No difference was noticeable on my Motorola X4, 5 second with screen with the DB version 37 and 5-6 seconds during the upgrade from 37 to 38. |
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.
Brilliant, thanks for checking the performance on mobile! It seems it's when migrating the "notes" table that we can get this white screen on startup. Other than that, there's just a minor tweak to make and we can merge.
packages/app-cli/tests/models_Tag.ts
Outdated
commonTags = await Tag.commonTagsByNoteIds([note0.id, note1.id, note2.id, note3.id]); | ||
|
||
let commonTagIds = commonTags.map(t => t.id); | ||
expect(commonTagIds.length).toBe(0); | ||
|
||
commonTags = await Tag.commonTagsByNoteIds([note1.id, note2.id, note3.id]); | ||
|
||
commonTagIds = commonTags.map(t => t.id); | ||
expect(commonTagIds.length).toBe(1); | ||
expect(commonTagIds.includes(taga.id)).toBe(true); | ||
|
||
commonTags = await Tag.commonTagsByNoteIds([note2.id, note3.id]); | ||
|
||
commonTagIds = commonTags.map(t => t.id); | ||
expect(commonTagIds.length).toBe(2); | ||
expect(commonTagIds.includes(taga.id)).toBe(true); | ||
expect(commonTagIds.includes(tagb.id)).toBe(true); | ||
|
||
commonTags = await Tag.commonTagsByNoteIds([note3.id]); | ||
|
||
commonTagIds = commonTags.map(t => t.id); |
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 a small thing, but you could you remove the white space changes please? Just to keep the diff and blame history simpler.
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 done
Looks good now, thanks @JackGruber! |
Count the completed todos at the tag sidebar based on the showCompletedTodos setting.