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

Create GroupSummaryStore for storing group summary stuff #1418

Merged
merged 4 commits into from
Sep 25, 2017

Conversation

lukebarnard1
Copy link
Contributor

  • 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
@lukebarnard1
Copy link
Contributor Author

Arguably this shouldn't use the client peg..

Copy link
Member

@dbkr dbkr left a 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(
Copy link
Member

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) {
Copy link
Member

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

@dbkr dbkr assigned lukebarnard1 and unassigned dbkr Sep 25, 2017
@lukebarnard1 lukebarnard1 assigned dbkr and unassigned lukebarnard1 Sep 25, 2017
@dbkr dbkr merged commit af2df77 into develop Sep 25, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants