-
-
Notifications
You must be signed in to change notification settings - Fork 832
Create GroupSummaryStore for storing group summary stuff #1418
Conversation
lukebarnard1
commented
Sep 22, 2017
- Acts as a layer between GroupView and the group APIs that modify the summary individually. This allows for abstraction of getting the new summary once a successful API hit has been done.
- The plan is to also control the avatar, topic, body of the summary via the same class
- Acts as a layer between GroupView and the group APIs that modify the summary individually. This allows for abstraction of getting the new summary once a successful API hit has been done. - The plan is to also control the avatar, topic, body of the summary via the same class
Arguably this shouldn't use the client peg.. |
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.
Otherwise lgtm, apart from you still havign a _loadGroupFromServer
in _onSaveClick
(which is really setGroupProfile
needing to go through the store) but looks like it should work fine as-is so this could be done later.
@@ -411,12 +430,16 @@ export default React.createClass({ | |||
}, | |||
|
|||
_loadGroupFromServer: function(groupId) { | |||
MatrixClientPeg.get().getGroupSummary(groupId).done((res) => { | |||
this._groupSummaryStore = new GroupSummaryStore( |
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.
Hmm, I'm assuming there's no references retained to the GroupSummaryStore so it will all GC nicely, but might be nice to explicitly remove the listener on the old one anyway.
@@ -411,12 +430,16 @@ export default React.createClass({ | |||
}, | |||
|
|||
_loadGroupFromServer: function(groupId) { |
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.
This feels like a bit of misnomer now since what's is mostly doing is initialising the store