Skip to content

Show validation results in the model editor#2997

Merged
robertbrignull merged 5 commits intomainfrom
robertbrignull/model-table-validation
Oct 19, 2023
Merged

Show validation results in the model editor#2997
robertbrignull merged 5 commits intomainfrom
robertbrignull/model-table-validation

Conversation

@robertbrignull
Copy link
Copy Markdown
Contributor

@robertbrignull robertbrignull commented Oct 18, 2023

Implements displaying validation in the model editor. Each validation result is added as a span 5 grid cell, so it sits underneath the modelings.

I wasn't sure whether the validation errors should span the full width or sit only underneath the modelings. I went with just under the modelings, but changing this would be very easy to do now or in the future.

For now I've disabled the "click on error to highlight model" functionality. I think it's less useful here than it is for the method modeling panel because all the modelings are visible at once and it's not that the invalid bit is on a different page. If we want to we could add highlighting of individual modelings, but it would add a little complexity.

Screenshot:

Screenshot 2023-10-19 at 13 01 11

Checklist

  • 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.

Base automatically changed from robertbrignull/model-table-multirow to main October 19, 2023 11:40
@robertbrignull robertbrignull force-pushed the robertbrignull/model-table-validation branch 2 times, most recently from 5c2b456 to fa185d9 Compare October 19, 2023 11:51
@robertbrignull robertbrignull force-pushed the robertbrignull/model-table-validation branch from fa185d9 to 0af77d0 Compare October 19, 2023 11:53
@robertbrignull robertbrignull marked this pull request as ready for review October 19, 2023 11:54
@robertbrignull robertbrignull requested a review from a team as a code owner October 19, 2023 11:54
@robertbrignull
Copy link
Copy Markdown
Contributor Author

I can add some tests, but this is the general idea of how the UI will look and behave.

Copy link
Copy Markdown
Member

@koesie10 koesie10 left a comment

Choose a reason for hiding this comment

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

LGTM, but adding a few tests and a story showing the validation errors would definitely be useful

type Props = {
error: ModeledMethodValidationError;
setSelectedIndex: (index: number) => void;
setSelectedIndex?: (index: number) => void;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It would be nice if we were able to highlight the model on which the error is located, but this can still be added in a follow-up PR if this is something that comes out of a design review.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I've opened a separate issue for this so we don't forget it.

@robertbrignull robertbrignull merged commit acb687c into main Oct 19, 2023
@robertbrignull robertbrignull deleted the robertbrignull/model-table-validation branch October 19, 2023 15:55
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.

2 participants