-
Notifications
You must be signed in to change notification settings - Fork 132
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
765 'ChatRoom' & 'ChatMessage' contexts and 'SendChatMessage' Intent #794
765 'ChatRoom' & 'ChatMessage' contexts and 'SendChatMessage' Intent #794
Conversation
✅ Deploy Preview for lambent-kulfi-cf51a7 ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
fix typo for roomId.
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.
LGTM, need another maintainer to also confirm.
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.
Added some suggestions to clarify a few things. Overall looks pretty good
docs/context/ref/ChatRoom.md
Outdated
const intentResolution = await fdc3.raiseIntent("StartChat", context); | ||
|
||
const chatRoom = intentResolution.getResult(): | ||
|
||
/* // The chatRoom will be like: | ||
chatRoom = { | ||
type: "fdc3.chat.room", | ||
providerName: "Symphony", | ||
id: { | ||
roomId: "j75xqXy25NBOdacUI3FNBH" | ||
} | ||
uri: "http://symphony.com/ref/room/j75xqXy25NBOdacUI3FNBH___pqSsuJRdA", | ||
name: 'My new room' | ||
}; | ||
*/ |
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.
I'd flip the order of the code and context example (to match convention on other pages). See https://fdc3.finos.org/docs/context/ref/Chart for an example
const intentResolution = await fdc3.raiseIntent("StartChat", context); | |
const chatRoom = intentResolution.getResult(): | |
/* // The chatRoom will be like: | |
chatRoom = { | |
type: "fdc3.chat.room", | |
providerName: "Symphony", | |
id: { | |
roomId: "j75xqXy25NBOdacUI3FNBH" | |
} | |
uri: "http://symphony.com/ref/room/j75xqXy25NBOdacUI3FNBH___pqSsuJRdA", | |
name: 'My new room' | |
}; | |
*/ | |
const chatRoom = { | |
type: "fdc3.chat.room", | |
providerName: "Symphony", | |
id: { | |
roomId: "j75xqXy25NBOdacUI3FNBH" | |
} | |
uri: "http://symphony.com/ref/room/j75xqXy25NBOdacUI3FNBH___pqSsuJRdA", | |
name: 'My new room' | |
}; | |
//Chat rooms are returned by the StartChat intent as a result | |
const intentResolution = await fdc3.raiseIntent("StartChat", context); | |
try { | |
const chatRoom = await intentResolution.getResult(): | |
} catch (error) { | |
//chat room was not created... | |
} |
docs/context/ref/ChatRoom.md
Outdated
--- | ||
# `ChatRoom` | ||
|
||
Reference to a chat room. |
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.
Add detail on why we need one (to send further messages to the room? to log in a CRM maybe?)
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.
Additional text added.
docs/context/ref/ChatRoom.md
Outdated
| `uri` | string | No | `'http://symphony.com/ref/room/j75xqXy25NBOdacUI3FNBH___pqSsuJRdA'` | | ||
| `name` | string | No | `'My new room'` | | ||
|
||
The `uri` is an universal url to access to the room. It could be opened from a browser, a mobile app, etc... |
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.
The `uri` is an universal url to access to the room. It could be opened from a browser, a mobile app, etc... | |
The `uri` field should be populated with a URL to access the room, which may be used outside of the context of a Desktop Agent (for example in a browser, mobile application etc.). |
Minor comment: is it expected to be a URL or URI? All URLs are URIs but not all URIs are URLs (although I don't know many people that are clear on the distinction). If its always expected to be retreivable in a browser (taking you to a web page where you can access teh item) rather than just acting as an identity, then its a URL (and should be named as such).
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.
Are we okay with the updated text @kriswest ?
@kriswest I updated the PR taking into account your remarks. Could you have a second look? Thanks |
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.
Have left some comments on some minor changes that may still be outstanding.
Are we still pending an updated description of use-cases for starting a chat?
|
Hi @mistryvinay
|
Currently we have:
Maybe something along the lines of:
|
Hi @mistryvinay |
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.
LGTM.
docs/context/ref/ChatRoom.md
Outdated
--- | ||
# `ChatRoom` | ||
|
||
Reference to a chat room. |
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.
Additional text added.
"type": { "const": "fdc3.chat.room" }, | ||
"providerName": { "type": "string" }, | ||
"id": { "type": "object" }, | ||
"uri": { "type": "string" }, |
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.
Does this need to be updated to url
now?
"type": { "const": "fdc3.chat.room" }, | ||
"providerName": { "type": "string" }, | ||
"id": { "type": "object" }, | ||
"uri": { "type": "string" }, |
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.
Same for this row, should this be updated to url
?
Add a CHANGELOG.md entry in the unreleased section please |
Hi @kriswest @mistryvinay |
FYI, this PR is included in new one #882 |
This PR:
See initial discussion #765