-
Notifications
You must be signed in to change notification settings - Fork 129
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
Support broadcast in FDC3 Action #1368
Support broadcast in FDC3 Action #1368
Conversation
✅ Deploy Preview for fdc3 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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.
Need to move some of the descriptive content into the schema and run the generator. See comments for details.
311548b
to
bd71172
Compare
Hi @kriswest |
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 looks good to me now.
I wasn't super clear in the previous comment about files to include - action.schema.json is the only file that needs to be touched manually. However, ContextTypes.ts should still make it into the PR - its generated on running npm run typegen
or npm run build
. Its setup to happen automatically on a precommit hook - but then it probably doesn't add the generated file to the commit.
Not a problem for merging this PR as it will happen alter before release.
Co-authored-by: Hugh Troeger <troeger.hugh@gmail.com>
f9fc79a
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.
Apologies spotted a small typo which lead to realising a part of the content needed a further tweak
Co-authored-by: Kris West <kris.west@interop.io>
@kriswest Thank you for your help. I committed your suggestion. |
Hi @symphony-jean-michael, looking good now - just needs a changelog entry adding: https://github.com/symphony-jean-michael/FDC3/blob/broadcast_in_action/CHANGELOG.md I would suggest something like the following (in the 'Added' section): - Added support for broadcast actions to the `fdc3.action` context type, allowing an Action to represent the broadcast of a specified context to an app or user channel. ([#1368](https://github.com/finos/FDC3/pull/1368)) You could also run We're going to add a PR template to the project soon with checkboxes for things like this to avoid slow back and forth on reviews like this ;-) |
Hi @kriswest |
Don't forget to push the change (not seeing it yet - but then github seems to be under particularly high load this morning so it may be them). Please add /src/ContextTypes.ts as well (should have changes after running |
@kriswest Yes it seems to take time. 😅 |
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
Some of the changes in the ContextTypes.ts file aren't great (types relating to SearchCriteria), but I don't think they relate to this PR. We'll work on those before releasing 2.2.
This PR applies the changes requested in the Enhancement Request: #1353