-
Notifications
You must be signed in to change notification settings - Fork 12
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
handle view field domain and defQueries issue #96
Conversation
if (JSON.stringify(field.domain) !== JSON.stringify(domainFields[i])) { | ||
// should mixin the update if the field already has some other update | ||
let hasUpdate: boolean = false; | ||
fieldUpdates.forEach((update: any) => { |
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 using some
instead of forEach
? That way, you can break out of the loop after the match is found.
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.
@MikeTschudi I'd not used that before but I like it...thanks! Will switch it up
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.
@MikeTschudi actually I still need to alter the array by setting the domain property on the instance where the name matches. I could still use a standard for loop and break out when the match is found though.
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 change would be to
fieldUpdates.some((update: any) => {
if (update.name === field.name) {
hasUpdate = true;
update.domain = domainFields[i];
}
return hasUpdate;
});
Wouldn't this accomplish the domain-setting for each matching newField?
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.
@MikeTschudi ahh..you are correct...could do it that way
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.
Up to you to change or not; please merge when you want
#90