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

Emit SubjectWrapper in verification grid decision-made event #295

Merged
merged 2 commits into from
Feb 27, 2025

Conversation

hudson-newey
Copy link
Member

@hudson-newey hudson-newey commented Feb 27, 2025

Emit SubjectWrapper in verification grid decision-made event

At the moment the verification grid decision-made event emits a tuple of [VerificationGridTile, DecisionModel]. This is poor form for multiple reasons such as:

  • The verification grid tile is an internal component, and should not be exposed to the user
  • Emitting a tuple is not ergonomic
  • Using the verification grid tile reference to get the subject model is error-prone. Because if the verification grid auto-pages before the subject model could be evaluated, it could change

Changes

  • The verification-grids decision-made event now emits a SubjectWrapper model which contains the tag, subject, verification, and classification models

Related Issues

Fixes: #294

Final Checklist

  • All commits messages are semver compliant
  • Added relevant unit tests for new functionality
  • Updated existing unit tests to reflect changes
  • Code style is consistent with the project guidelines
  • Documentation is updated to reflect the changes (if applicable)
  • Link issues related to the PR
  • Assign labels if you have permission
  • Assign reviewers if you have permission
  • Ensure that CI is passing
  • Ensure that pnpm lint runs without any errors
  • Ensure that pnpm test runs without any errors

@hudson-newey hudson-newey added enhancement New feature or request Architecture ecosounds Work needed for host application integration labels Feb 27, 2025
@hudson-newey hudson-newey self-assigned this Feb 27, 2025
Copy link

github-actions bot commented Feb 27, 2025

Copy link
Contributor

@atruskie atruskie left a comment

Choose a reason for hiding this comment

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

this is a breaking change - make sure you do whatever it is that's needed to do a major version bump

double check if we can type the emitted event

@hudson-newey
Copy link
Member Author

image

Event is now typed in the custom-elements.json, in the HTMLElementTagNameMap, and on the doc site. I have included it as a separate commit named "docs: Add event typings to documentation site" in an attempt to keep commits clean

@hudson-newey hudson-newey force-pushed the verbose-decision-models branch from d16405f to 62d8a21 Compare February 27, 2025 01:49
@hudson-newey hudson-newey merged commit bb257f9 into main Feb 27, 2025
3 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Architecture ecosounds Work needed for host application integration enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

decision-made event should emit SubjectWrapper
2 participants