Skip to content
This repository has been archived by the owner on Nov 8, 2018. It is now read-only.

show total count for drafts folder #1396

Merged
merged 2 commits into from
Apr 3, 2016
Merged

Conversation

tahaalibra
Copy link
Contributor

this solves issue #540

Currently the drafts shows unseen/unread count. this solution would enable to show total count for the drafts folder only.

review @Gomez @ChristophWurst, This is my first PR, i am open for code review

@mention-bot
Copy link

By analyzing the blame information on this pull request, we identified @ChristophWurst, @Gomez and @jancborchardt to be potential reviewers

@@ -10,7 +10,7 @@
<a class="folder {{#if specialRole}} icon-{{specialRole}} svg{{/if}}">
{{name}}
{{#if unseen}}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We go in only if unseen. Drafts are always seen. So you need to do further tests here.

@Gomez
Copy link
Contributor

Gomez commented Apr 1, 2016

Thanks for this PR! Please add a changelog enty, too.

@ChristophWurst
Copy link
Contributor

@tahaalibra thanks for that PR. I'd prefer to have that view logic inside the folder view. That can be achieved easily by using template helpers like we did here.

Using a view helper is a valid approach, but it makes the code harder to maintain as you'd have the logic split into two separate locations. There's only one template that would make use of it.

So simply move the filter code over as template helper and fix the bug @Gomez mentioned, thanks!

@tahaalibra tahaalibra force-pushed the master branch 2 times, most recently from 855befb to 56e0b3d Compare April 1, 2016 18:47
show total count for drafts
@tahaalibra
Copy link
Contributor Author

Updated the code...Using template helpers seems much better option

@ChristophWurst
Copy link
Contributor

Thanks a lot, that looks very good!

We'll have to decrement that 'total' number when the user deletes a message. Otherwise the counter stays unchanged while deleting drafts. There's code that updates the 'unseen' value, so we just have to adjust that to also update the other value.
@tahaalibra we can either do that in this PR or a follow-up PR. Do you want to have a look at that?

@ChristophWurst
Copy link
Contributor

This is where clicks on the trash icon are handled. You can use require('state').currentFolder to get a reference to the folder model, where the 'total' counter has to be decreased. Changes to that property should trigger a re-render of the folder view, but I'm not a 100% sure about that. One has to try ;-)

@tahaalibra
Copy link
Contributor Author

i think this also need to be updated for another case i.e if a user receives a new message...

@tahaalibra
Copy link
Contributor Author

Also the title bar still shows drafts with unseen....i will try to correct these problem...i will contact if i need any help

@tahaalibra
Copy link
Contributor Author

[x] the count decreases if we delete a message in the drafts folder
[x] the title of the page now shows total count instead of unseen. this is for drafts folder only

one thing though, i checked this without this PR, the unseen count for the index folder does not if suppose the user has loaded the mail page, then he receives a new mail, then he clicks 'check messages'. the unseen message count does not get updated.

@Gomez
Copy link
Contributor

Gomez commented Apr 3, 2016

Thanks @tahaalibra! Works now.

When i create further drafts, the counter does not reflect this immediately. A complete reload is needed. I will create a issue for it.

The "unseen count for the inbox folder" is a known issue. There is a hacky (closed) PR #1047. Will create a further issue (as we dont have one).

@Gomez Gomez closed this Apr 3, 2016
@Gomez Gomez reopened this Apr 3, 2016
@Gomez Gomez merged commit 29a0519 into owncloud-archive:master Apr 3, 2016
@Gomez
Copy link
Contributor

Gomez commented Apr 3, 2016

@ChristophWurst And thanks for the explanation! Greatly appreciated!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants