Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Implement Migration Complexity Explanation providing the summary and rationale for reported complexity #2166
Implement Migration Complexity Explanation providing the summary and rationale for reported complexity #2166
Changes from 10 commits
bf65641
028c168
730da79
cbbf84f
9869d5c
de90d2c
988fae3
4ac3167
c36b10a
df9ea11
25b150b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would like to avoid having HTML in multiple places. It should all ideally be in the template file. Morevoer, field
AssessmentReport.MigrationComplexityExplanation
having HTML would be weird.But I get why you did this. (because json and HTML have different requirements).
Here's one way to do this:
AssessmentReport
, instead of storing just the final string inMigrationComplexityExplanation
, store the actualExplanationData
struct.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree to this, we should keep all the HTML code in that template
yes we can keep it in JSON as well, in case of tests and all you are anyways ignoring it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added a TODO comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about
Below is a breakdown of the issues detected in different categories for each impact level
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of using this function we can get the Feature heading for that Enum to be consistent with the heading in report.
but given that we are anyways changing the UI, we can revisit it later so okay for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah lets revisit this. The category here is being fetched from the analyze schema issue.
I see the same issues of formatting in schema reports. We need to fix the root.