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

1255 Make fdc3.fileAttachment an independent type #1261

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Yannick-Malins
Copy link
Contributor

Copy link

netlify bot commented Jul 15, 2024

Deploy Preview for fdc3 ready!

Name Link
🔨 Latest commit 92ecb7a
🔍 Latest deploy log https://app.netlify.com/sites/fdc3/deploys/670f678646cd4d0008e25ca9
😎 Deploy Preview https://deploy-preview-1261--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.

@Yannick-Malins
Copy link
Contributor Author

@kriswest

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 - although I think Message's entities element needs to be defined with oneOf rather than anyOf. Could you change that, add a Changelog entry and then I'll approve and merge the change (new governance restrictions will invalidate any reviews that happen before a later change)

@kriswest kriswest assigned Yannick-Malins and unassigned kriswest Jul 29, 2024
@kriswest kriswest requested a review from a team July 29, 2024 09:26
@kriswest kriswest changed the title 1255 - split off fileAttachment 1255 Make fdc3.fileAttachment and independent type Sep 3, 2024
@kriswest
Copy link
Contributor

@Yannick-Malins lemme know if you can make these two quick changes (I didn't add suggestions or I won't be able to review the change after you merge them) - alternatively let me know to re-raise this PR and have you review instead.

@kriswest kriswest added this to the 2.2 candidates milestone Sep 18, 2024
@kriswest kriswest changed the title 1255 Make fdc3.fileAttachment and independent type 1255 Make fdc3.fileAttachment an independent type Oct 9, 2024
@kriswest
Copy link
Contributor

kriswest commented Oct 9, 2024

Todo:

  • the filename and this $id need to match in fileAttachment.schema.json
  • define message entities with oneOf rather than anyOf in message.schema.json
  • add a changelog

@Yannick-Malins Yannick-Malins requested a review from a team as a code owner October 11, 2024 09:38
@kriswest
Copy link
Contributor

Thanks @Yannick-Malins. I'm not seeing the new type in the preview - but that could be because the PR is '199 commits behind finos/FDC3:main'. Could you merge changes from main then run npm run build. That should re-generate ContextTypes.ts so that goes in and the preview should generate correctly afterwards (those two are unrelated but we need to check both).

Otherwise looks all good to me. I'll get around to adding a PR template soon so the above steps are documented!

@Yannick-Malins
Copy link
Contributor Author

should be good now!

@kriswest
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

fdc3.entity.fileAttachment is only defined inline in the fdc3.message schema and example has wrong type
4 participants