Skip to content
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

Actually use diffTracker values for display to the user. #3375

Merged

Conversation

rtibbles
Copy link
Member

Summary

Description of the change(s) you made

  • Makes the use of the diffTracker to generate getter values actually find the values in the diffTracker (where they are keyed by node id, and then the key for the property) rather than just keying directly by the property
  • This should fix the apparent editing lag that was observed by @sairina in her recently merged Edit modal - add accessibility field #3366
  • This issue was originally introduced in 599e338

Manual verification steps performed

  1. Check an accessibility checkbox
  2. See it be immediately checked

@rtibbles rtibbles requested a review from sairina April 19, 2022 00:34
let results = uniq(this.nodes.map(node => node[key] || null));
const results = uniq(
this.nodes.map(node => {
if (Object.prototype.hasOwnProperty.call(this.diffTracker[node.id] || {}, key)) {
Copy link
Contributor

@sairina sairina Apr 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiosity, why use Object.prototype.hasOwnProperty.call()? Does this have to do the this.diffTracker[node.id] object having to use call from the prototype?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hasOwnProperty is the best method for determining if a Javascript object actually has a property in itself (and it's not inherited from a parent type) - so it's the best way of telling if we have actually assigned e.g. this.diffTracker[node.id] = {}.

In theory you could call it like this: this.diffTracker.hasOwnProperty(node.id) because it is just a method of any Object that inherits from or is instantiated from the Object constructor. The reason for using the Object.prototype.hasOwnProperty.call is to ensure that the method has not been maliciously overwritten on the object itself - we have a linting rule https://eslint.org/docs/rules/no-prototype-builtins that guards against this - by calling it directly from the Object prototype we guard against it having been overwritten from JSON data or somesuch. It is probably not a real concern in this instance, but simpler to obey the linting rule.

Copy link
Contributor

@sairina sairina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Manual testing works for the accessibility checkboxes!

@rtibbles rtibbles merged commit ec3b805 into learningequality:unstable Apr 20, 2022
@rtibbles rtibbles deleted the the_diff_that_never_was branch April 20, 2022 16:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants