Skip to content

Conversation

@charisk
Copy link
Contributor

@charisk charisk commented Mar 19, 2024

The ModelAlerts view won't return any HTML if the variantAnalysis isn't set, so I've updated the component to be able to receive that, along with repo states and repo results, through props. We follow this pattern elsewhere.

Checklist

N/A:

  • CHANGELOG.md has been updated to incorporate all user visible changes made by this pull request.
  • Issues have been created for any UI or other user-facing changes made by this pull request.
  • [Maintainers only] If this pull request makes user-facing changes that require documentation changes, open a corresponding docs pull request in the github/codeql repo and add the ready-for-doc-review label there.

@charisk charisk requested a review from a team as a code owner March 19, 2024 09:24
@charisk charisk mentioned this pull request Mar 19, 2024
3 tasks
Copy link
Contributor

@norascheuch norascheuch left a comment

Choose a reason for hiding this comment

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

Was this noticeable in the extension itself? Your changes are in the general ModelAlerts view, did we have a bug?

@charisk
Copy link
Contributor Author

charisk commented Mar 19, 2024

Was this noticeable in the extension itself? Your changes are in the general ModelAlerts view, did we have a bug?

No there's no bug (also reminder that this view is for a prototype, not a released thing, so it probably has lots of "bugs" 😬 ).

It's just that the component returns empty HTML if variant analysis isn't set so the story is kind of pointless without it.

The changes to the view make it possible to make it easily "storyable".

Copy link
Contributor

@norascheuch norascheuch left a comment

Choose a reason for hiding this comment

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

🚢

@charisk charisk merged commit 724370f into main Mar 19, 2024
@charisk charisk deleted the charisk/fix-model-alerts-stories branch March 19, 2024 16:07
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.

3 participants