-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Add desktop dock icon indicator in Electron #525
Conversation
src/page/home/sidebar/SidebarLink.js
Outdated
}, | ||
}), | ||
)(SidebarLink); | ||
export default SidebarLink; |
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.
Just cleaning this up. I couldn't really see any reason why we should want to add both the withRouter
and withIon
when we can just get the info from props.
I guess this is more a philosophical question, but should we be implementing the mobile count as well in this PR? |
I'm up for looking into that! I don't think there's any rush here and that would definitely hook into this same logic I'd imagine. |
But to answer philosophically I wouldn't stress about having implemented something like this for only one platform as long as there are plans to eventually release a feature for all applicable platforms. Once this app is in people's hands I'd vote for sticking more closely to that philosophy, but have it be a flexible rule since we could end up with some pretty mondo PRs. |
Looks like iOS has this out of the box. Android not so much. Another thing to consider is that this update might need to happen as a response to a push notification and not a pusher update while the app is open. |
Alright, added a way to set the app badge icon in iOS, but can't test it since I don't have a working iOS device at the moment. |
@marcaaron lmk when it's ready and I can test for you 👍 |
Go for it @roryabraham ! |
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.
Nice cleanup on the sidebarlink. Just a couple of small things here.
metro.config.js
Outdated
@@ -5,6 +5,7 @@ | |||
* @format | |||
*/ | |||
|
|||
|
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.
Unnecessary extra space
src/lib/actions/Report.js
Outdated
// There are unread items if the last one the user has read is less | ||
// than the highest sequence number we have. | ||
const unreadActionCount = (maxSequenceNumber || report.reportActionList.length) - usersLastReadActionID; | ||
return unreadActionCount > 0 ? unreadActionCount : 0; |
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 think using Math.max()
here makes it more readable.
Just coming back to this one. This PR has gotten a little big and I've uncovered a few bugs in the existing code that I've fixed here... so, I'm going to split this into two PRs so we can focus on making sure that:
|
Putting a hold on this until we merge #597 |
Taking hold off this one now that half these changes have been merged :) |
Fix conflicts on this one and ready for another review. |
Fixes: https://github.com/Expensify/Expensify/issues/141720
cc @tgolen who probably has some ideas on how to improve this...
In order to accomplish this I added an
unreadActionCount
to reports in addition toisUnread
this way we can pass a number to anything that wants to know about how many unread there are.Other things I'd like to do:
Move theseDoneipcRenderer
events into some kind ofELECTRON_EVENTS
fileDebounce theDoneupdatePageTitleAndUnreadCount()
method since it gets called many times in quick succession when anything subscribes toIONKEYS.COLLECTION.REPORT
(maybe there's another way around this - but seems to work ok)Tests:
1. Reboot the app and verify that the icon is updated correctly 1. Click on one of the unread chats 1. Verify that the dock icon number clears
1. Nav away from the chat and add two more messages 1. Verify the dock icon count increments to 2