Skip to content
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

Merged
merged 5 commits into from
Oct 14, 2024

Conversation

symphony-jean-michael
Copy link
Contributor

This PR applies the changes requested in the Enhancement Request: #1353

@symphony-jean-michael symphony-jean-michael requested a review from a team as a code owner September 23, 2024 13:34
Copy link

linux-foundation-easycla bot commented Sep 23, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

Copy link

netlify bot commented Sep 23, 2024

Deploy Preview for fdc3 ready!

Name Link
🔨 Latest commit 4f34a71
🔍 Latest deploy log https://app.netlify.com/sites/fdc3/deploys/670cee97044cae000887c496
😎 Deploy Preview https://deploy-preview-1368--fdc3.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Contributor

@kriswest kriswest left a 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.

docs/context/ref/Action.md Outdated Show resolved Hide resolved
docs/context/ref/Action.md Outdated Show resolved Hide resolved
docs/context/ref/Action.md Outdated Show resolved Hide resolved
docs/context/ref/Action.md Outdated Show resolved Hide resolved
docs/context/ref/Action.md Outdated Show resolved Hide resolved
@symphony-jean-michael
Copy link
Contributor Author

Hi @kriswest
Thank you for your help.
I've modified the PR according to your comments. Now only the action.schema.json file has been updated.

kriswest
kriswest previously approved these changes Sep 26, 2024
Copy link
Contributor

@kriswest kriswest left a 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.

@kriswest kriswest requested a review from a team September 26, 2024 13:10
bingenito
bingenito previously approved these changes Sep 26, 2024
@kriswest kriswest linked an issue Sep 27, 2024 that may be closed by this pull request
Copy link
Contributor

@kriswest kriswest left a 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

schemas/context/action.schema.json Outdated Show resolved Hide resolved
@symphony-jean-michael
Copy link
Contributor Author

@kriswest Thank you for your help. I committed your suggestion.

bingenito
bingenito previously approved these changes Oct 9, 2024
Co-authored-by: Hugh Troeger <[email protected]>
@kriswest
Copy link
Contributor

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 npm run build in the root of the project before checking in so that ContextTYpes.ts gets regenerated.

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 ;-)

@symphony-jean-michael
Copy link
Contributor Author

Hi @kriswest
I updated the Changelog file. :)

@kriswest
Copy link
Contributor

Hi @kriswest I updated the Changelog file. :)

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 npm run build).

@symphony-jean-michael
Copy link
Contributor Author

@kriswest Yes it seems to take time. 😅
Screenshot 2024-10-14 at 11 40 58

Copy link
Contributor

@kriswest kriswest left a 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.

@kriswest kriswest merged commit 93685d9 into finos:main Oct 14, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support broadcast in FDC3 Action
4 participants