Skip to content

Conversation

@segheysens
Copy link

@segheysens segheysens commented Aug 11, 2022

Contributing to Twilio

All third-party contributors acknowledge that any contributions they provide will be made under the same open-source license that the open-source project is provided under.

  • I acknowledge that all my contributions will be made under the project's license.

private(set) var observableConversation: ObservableFetchRequestResult<PersistentConversationDataItem>?
lazy private(set) var observableMessageList = conversationsRepository.getObservableMessages(for: conversationSid)
private(set) var observervableTypingMemeberList: ObservableFetchRequestResult<PersistentParticipantDataItem>?
private(set) var observervableTypingMemberList: ObservableFetchRequestResult<PersistentParticipantDataItem>?
Copy link
Contributor

Choose a reason for hiding this comment

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

observableTypingParticipantList sounds more clear

@berkus berkus assigned berkus and segheysens and unassigned berkus Sep 1, 2022
@berkus berkus added the documentation Improvements or additions to documentation label Sep 1, 2022
weak var delegate: ConversationViewModelDelegate?

// MARK: Init
// Initialize the Conversation
Copy link

Choose a reason for hiding this comment

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

Keep the "MARK:" prefix - it shows up in Xcode code minimap.

messagesManager.reactToMessage(withSid: sid, withReaction: reaction)
}

// MARK: Deintialization
Copy link

Choose a reason for hiding this comment

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

Same for these.

@berkus
Copy link

berkus commented Sep 1, 2022

@segheysens hi, could you please address the review comments?

@segheysens
Copy link
Author

@berkus made these updates!

}

// MARK: Deintialization
// Deinitialization
Copy link
Contributor

Choose a reason for hiding this comment

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

Return MARK here please.

}

// MARK: Methods
// Methods
Copy link
Contributor

Choose a reason for hiding this comment

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

And here please, too.

observableConversation?.removeObserver(self)
observableMessageList.removeObserver(self)
observervableTypingMemeberList?.removeObserver(self)
observervableTypingMemberList?.removeObserver(self)
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't it be observableTypingParticipantList?

@berkus
Copy link

berkus commented Oct 6, 2022

@segheysens hi! Please address the remaining notes and I believe the observableTypingParticipantList rename is not finished - there are still old names in a few places below. Please double check by compiling the app!

@berkus
Copy link

berkus commented Dec 5, 2022

@segheysens hi, do you plan to finish up this PR?

@berkus
Copy link

berkus commented Sep 28, 2023

@segheysens gentle poke!

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

Labels

documentation Improvements or additions to documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants