Skip to content
This repository has been archived by the owner on Mar 13, 2024. It is now read-only.

[MM 7970] Support maintaining online status while the Desktop App is in the background ... and other things. #2992

Merged
merged 2 commits into from
Jun 21, 2019
Merged

[MM 7970] Support maintaining online status while the Desktop App is in the background ... and other things. #2992

merged 2 commits into from
Jun 21, 2019

Conversation

deanwhillier
Copy link
Contributor

@deanwhillier deanwhillier commented Jun 20, 2019 •
edited
Loading

Summary

See Desktop App PR for details.

Related Pull Requests

Desktop: MM-7970
Webapp: MM-7970 (this PR)
Server: MM-14582

@deanwhillier deanwhillier added the Work in Progress Not yet ready for review label Jun 20, 2019
This was referenced Jun 20, 2019
Merged
Merged
@deanwhillier deanwhillier added 2: Dev Review Requires review by a core commiter and removed Work in Progress Not yet ready for review labels Jun 20, 2019
deanwhillier
deanwhillier commented Jun 20, 2019
View reviewed changes
utils/webapp_connector.js
@@ -0,0 +1,19 @@
// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lieut-data, you had previously suggested that this could be owned by the Desktop App. Were you thinking it was okay for this to not even exist unless the Desktop App loaded the Webapp and then just wrap any calls to it from the Webapp in a conditional before trying to use?

Copy link
Member

Choose a reason for hiding this comment

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

0/5 on the past suggestion -- it's nice to have the connector be permanently present and avoid inline conditionals, so this seems fine.

jwilander
jwilander approved these changes Jun 20, 2019
View reviewed changes
Copy link
Member

@jwilander jwilander left a comment

Choose a reason for hiding this comment

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

LGTM

lieut-data
lieut-data approved these changes Jun 20, 2019
View reviewed changes
utils/webapp_connector.js Outdated Show resolved Hide resolved
utils/webapp_connector.js
// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved.
// See LICENSE.txt for license information.

/**
Copy link
Member

Choose a reason for hiding this comment

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

nit: is the multi-line comment necessary (vs. extra // as necessary).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used ESDoc conventions on new files I created, this one included. Can convert if you'd prefer.

utils/webapp_connector.js
@@ -0,0 +1,19 @@
// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved.
Copy link
Member

Choose a reason for hiding this comment

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

0/5 on the past suggestion -- it's nice to have the connector be permanently present and avoid inline conditionals, so this seems fine.

@deanwhillier deanwhillier added this to the v5.14.0 milestone Jun 20, 2019
@deanwhillier deanwhillier added 4: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core commiter labels Jun 21, 2019
@deanwhillier deanwhillier merged commit dfff668 into master Jun 21, 2019
@deanwhillier deanwhillier deleted the MM-7970-user-status-monitoring branch June 21, 2019 14:54
@amyblais amyblais added Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation labels Jul 5, 2019
@amyblais amyblais added Docs/Needed Requires documentation and removed Docs/Not Needed Does not require documentation labels Sep 23, 2019
@lindy65 lindy65 added Tests/Done Release tests have been written and removed 4: Reviews Complete All reviewers have approved the pull request labels Sep 24, 2019
@esethna esethna added Docs/Not Needed Does not require documentation and removed Docs/Needed Requires documentation labels Oct 10, 2019
@esethna
Copy link
Contributor

esethna commented Oct 10, 2019

Docs covered by another PR

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@lieut-data lieut-data lieut-data approved these changes

@jwilander jwilander jwilander approved these changes

Assignees

@esethna esethna

Labels
Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation Tests/Done Release tests have been written
Projects
None yet
Milestone
v5.14.0
6 participants
@deanwhillier @esethna @lieut-data @jwilander @lindy65 @amyblais