-
Notifications
You must be signed in to change notification settings - Fork 2
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
Feat/clearer diffs #1910
Feat/clearer diffs #1910
Conversation
473f4ad
to
a58d853
Compare
a5d14ec
to
32bd1d3
Compare
…will change (no states in between)
…statuses in project form summary
…SummaryReportFormSummary
32bd1d3
to
62785eb
Compare
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.
Slick!! This is a bunch of fantastic work.
Just nitpicking about a couple of comments, we don't have to fix them
@@ -65,6 +67,12 @@ const AttachmentTableRow: React.FC<Props> = ({ | |||
return ( | |||
<> | |||
<tr> | |||
{operation && ( | |||
<td> | |||
{operation.charAt(0).toUpperCase() + |
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.
Nice! This will help clarify for our users
formDataTableName: "reporting_requirement" | ||
reportType: "TEIMP" |
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.
This was a bug the whole time, right?
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.
It was, so these would always have been showing up as new and the previous data in tests was always empty
}, | ||
latestCommittedReportMap[ | ||
milestoneReport.newFormData.reportingRequirementIndex | ||
] |
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.
Sweet, I think this was missing in the first place right?
proponentsSharePercentage: 10, // will trigger three diffs | ||
totalProjectValue: 12, // will trigger three diffs |
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.
Comment needs adjusting, we don't display three diffs anymore
This PR updates the way the custom diff fields represent the changes. The fields will now show the latest committed state (if any), and what will be changed/added/removed in the committing project revision. The intent is that it will be easier for users to discern what they will be changing about a project when applying a revision by avoiding showing them any intermittent states the project may have been in between the revision being created, and now.