-
Notifications
You must be signed in to change notification settings - Fork 14
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
#3815 - Identification of changed information during edit #4221
#3815 - Identification of changed information during edit #4221
Conversation
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.
No changes in these configurations, they were extracted from the packages.json as they were.
{ | ||
"type": "node", | ||
"request": "launch", | ||
"name": "Unit tests - Current test file", |
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.
👍
sources/packages/backend/apps/api/src/services/application/application.service.ts
Show resolved
Hide resolved
); | ||
// If there is a previous application, generate its read-only data. | ||
const previousReadOnlyDataPromise = | ||
previousApplicationVersion && |
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.
👍
...ackages/backend/apps/api/src/route-controllers/application/application.controller.service.ts
Show resolved
Hide resolved
const dataChangeDTO: ApplicationDataChangeAPIOutDTO = { | ||
key: dataChange.key, | ||
index: dataChange.index, | ||
changeType: dataChange.changeType, |
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.
Suggestion:
add changes to the const dataChangeDTO:
changes: dataChange.changes.length ? [] : undefined
and do the below changes,
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.
I am not able to see the benefit. Would it be to improve the code readability or something else?
Unless there is a stronger reason I am keeping the current way.
...ackages/backend/apps/api/src/route-controllers/application/application.controller.service.ts
Outdated
Show resolved
Hide resolved
@@ -16,6 +16,7 @@ | |||
{{ emptyStringFiller(applicationDetail.applicationNumber) }} | |||
</h2> | |||
<StudentApplication | |||
@render="formRender" |
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.
👍
if ( | ||
component.type === FromIOComponentTypes.Hidden || | ||
component._visible === false |
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.
👍
|
||
.changed-value::before { | ||
@extend .changed-value-content-base; | ||
content: "\00a0\00a0Updated"; |
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.
What is this \00a0\00a0
?
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.
Just to add some white space.
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.
Can you try like this ?
content: " Updated";
white-space: pre;
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 looks better, I did the change 😉
Great work @andrewsignori-aot and thanks for the walkthrough. I see that almost all the comments are addressed. Only one question and clarification there 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.
Thanks for doing the changes. 👍Great work.
Quality Gate passedIssues Measures |
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.
LGTM, nice and great work @andrewsignori-aot
New method
compareApplicationData
JSON.stringify
a simple solution to perform a deep equality between two objects. During the tests, the comparison of two part-time applications took less than a millisecond to execute.Form.io considerations
render
was used instead ofload
because the lists are not fully created right after the loading stage which means that not all the children will be present to have the highlight style applied.document.getElementById
was used to ensure that a child component in a list would receive the highlight style.Other changes
Note: The
compareApplicationData
would also be able to detect changes in the non-read-only application dynamic data.Sample API result
Sample UI Changes
Non-PR-related changes
Unit tests - Current test file
following the same idea from other available.--config
while executing thenpm commands
, following the same idea from the E2E tests.