Skip to content
This repository was archived by the owner on Nov 5, 2025. It is now read-only.

Comments

Resolve reviews: typed public AppEmbeddedSDK methods and add documentations for them#147

Merged
graywolf336 merged 4 commits intoRocketChat:alphafrom
shiqimei:draft-alpha.IUserInfo-IRoomInfo
Oct 2, 2019
Merged

Resolve reviews: typed public AppEmbeddedSDK methods and add documentations for them#147
graywolf336 merged 4 commits intoRocketChat:alphafrom
shiqimei:draft-alpha.IUserInfo-IRoomInfo

Conversation

@shiqimei
Copy link
Contributor

@shiqimei shiqimei commented Oct 1, 2019

Resolve: #145 (comment)
Resolve: #145 (comment)
Resolve: #145 (comment)

@shiqimei shiqimei mentioned this pull request Oct 1, 2019
@codecov
Copy link

codecov bot commented Oct 1, 2019

Codecov Report

Merging #147 into alpha will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##            alpha     #147   +/-   ##
=======================================
  Coverage   59.65%   59.65%           
=======================================
  Files          73       73           
  Lines        2377     2377           
  Branches      350      350           
=======================================
  Hits         1418     1418           
  Misses        959      959

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b3f6979...9230515. Read the comment docs.

@d-gubert d-gubert requested a review from graywolf336 October 1, 2019 18:17
Copy link
Contributor

@graywolf336 graywolf336 left a comment

Choose a reason for hiding this comment

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

Would also like to see the class documentation added, that way people can see whether it's for the client or the server alongside the naming convention.

@@ -0,0 +1,17 @@
import { IUserInfo } from './IUserInfo';

export interface IRoomInfo {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's call these something client side specific, otherwise developers working on the backend/server side could easily get confused as to which one to use.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great practice! Learned a lot 😃

@@ -0,0 +1,14 @@
export interface IUserInfo {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's call these something client side specific, otherwise developers working on the backend/server side could easily get confused as to which one to use.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed

* rename to make sure others can see that it's for client-side
* add the class documentation for the AppClientEmbeddedSDK
* Rename to make sure others can see it's for client-side
* Add documentation for the IClientRoomInfo and IClientInfo interfaces
Copy link
Contributor

@graywolf336 graywolf336 left a comment

Choose a reason for hiding this comment

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

Awesome, looking forward to this!

@graywolf336 graywolf336 merged commit a3a22be into RocketChat:alpha Oct 2, 2019
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