-
Notifications
You must be signed in to change notification settings - Fork 10
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
fix(applicant): add more logic to disable/enable save and close button #2918
Conversation
20c5edc
to
98449dd
Compare
API branch is deployed to platta: https://helsinkilisa-pr2918.api.dev.hel.ninja/healthz 🚀🚀🚀 |
HANDLER branch is deployed to platta: https://helsinkilisa-ui-handler-pr2918.dev.hel.ninja 🚀🚀🚀 |
APPLICANT branch is deployed to platta: https://helsinkilisa-ui-pr2918.dev.hel.ninja 🚀🚀🚀 |
TestCafe result is success for https://helsinkilisa-ui-handler-pr2918.dev.hel.ninja 😆🎉🎉🎉 |
TestCafe result is success for https://helsinkilisa-ui-pr2918.dev.hel.ninja 😆🎉🎉🎉 |
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.
Functionally, seems good. Do consider using the given suggestion for the type woes before merging.
type FlattenedObject = Record<string, string>; | ||
|
||
function flattenObject( | ||
obj: FlattenedObject, | ||
parentKey = '', | ||
result: FlattenedObject = {} | ||
): FlattenedObject { | ||
const appendResult = result; | ||
Object.keys(obj).forEach((key) => { | ||
const newKey = `${parentKey}${parentKey ? '.' : ''}${key}`; | ||
if ( | ||
typeof obj[key] === 'object' && | ||
obj[key] !== null && | ||
!Array.isArray(obj[key]) | ||
) { | ||
flattenObject(obj[key] as unknown as FlattenedObject, newKey, result); | ||
} else { | ||
appendResult[newKey] = obj[key]; | ||
} | ||
}); | ||
return result; | ||
} |
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.
The implementation is sound but the typing is iffy, yeah. The input is already assigned to FlattenedObject – assumed to be an object with only string values – which doesn't make sense since the whole point of the function is to flatten nested objects onto one level.
Here's how I would type this. The effective FormikTouched and FormikErrors types already are the same shape as the SimpleObject type given, which means the casts in the next function can also be removed altogether.
type SimpleObjectNode = SimpleObject | Array<SimpleObjectNode> | string | number | boolean | null;
type SimpleObject = { [prop: string]: SimpleObjectNode };
function isSimpleObject(a: SimpleObjectNode): a is SimpleObject {
return typeof a === 'object' && a !== null && !Array.isArray(a);
}
function flattenObject(
obj: SimpleObject,
parentKey = '',
result: { [prop: string]: Exclude<SimpleObjectNode, SimpleObject> } = {}
): SimpleObjectNode {
const appendResult = result;
Object.keys(obj).forEach((key) => {
const newKey = `${parentKey}${parentKey ? '.' : ''}${key}`;
const item = obj[key];
if (isSimpleObject(item)) {
flattenObject(item, newKey, result);
} else {
appendResult[newKey] = item;
}
});
return result;
}
In our use case, where we only care about the object-ness of each leaf and the keys of the output, we could even define SimpleObjectNode as SimpleObject | unknown
and I believe it would still work.
Object.keys(flattenObject(touched as FlattenedObject)).filter((field) => | ||
Object.keys(flattenObject(errors as FlattenedObject)).some((errorField) => | ||
errorField.includes(field) |
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.
With the above suggestion, these could just be passed as is.
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.
Oh, and flattenObject
is being run for errors quite a few times if there are many touched fields. I doubt there's much of a reason to fear performance issues here, but the flattened objects could also be optionally assigned to variables first before this two-level lookup happens.
Quality Gate passed for 'yjdh'Issues Measures |
Quality Gate passed for 'yjdh'Issues Measures |
Quality Gate passed for 'yjdh'Issues Measures |
Quality Gate passed for 'yjdh'Issues Measures |
API branch is deployed to platta: https://helsinkilisa-pr2918.api.dev.hel.ninja/healthz 🚀🚀🚀 |
HANDLER branch is deployed to platta: https://helsinkilisa-ui-handler-pr2918.dev.hel.ninja 🚀🚀🚀 |
TestCafe result is success for https://helsinkilisa-ui-handler-pr2918.dev.hel.ninja 😆🎉🎉🎉 |
Description ✨
Save and close button was disabled if any field on dataset
employee
orcompany
was changed to an invalid value.Logic is as follows:
Rather than trying to compare
touched
, a single field, against aerrors
data set:errors
object contains.@EmiliaMakelaVincit if you know TypeScript any better then feel free to format the function to a more prettier one. This is the best I can do 😓