Simplify annotation component by removing duplicated state #8
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
066db8f to
f3ab447
Compare
[Bug]New annotations not moved to current group when switching groups #3380f3ab447 to
07c33b4
Compare
|
hypothesis/h#3559 doesn't seem to be fixed, still seeing it |
|
Found one issue that I can reproduce on this branch but not on master:
|
Indeed. I realized I missed out the fix for that. I think that would be best put in a separate PR after this one is merged. I've removed #3559 from the description. |
As a step towards making <annotation> stateless or nearly-stateless and instead just a display of an annotation derived from the current annotation instance and its draft, remove the `domainModel` copy of `vm.annotation`.
Instead of maintaining a state flag indicating whether an annotation is being viewed, edited or created, derive that state from whether the annotation has an ID and whether it has an unsaved draft or not. In the process this commit simplifies the tests for reverting edits and adds a missing test that the annotation is deleted if new when clicking the Cancel button.
These variables duplicated the information from the current draft for an annotation. Instead of restoring a draft when an annotation card is created and saving it when the annotation card is destroyed, just update the draft whenever the tags, text or privacy of the annotation is changed. The text/tags/privacy shown in the card are determined by `vm.state()` which merges the annotation and the draft. Removing these variables lets us also remove or simplify the tests that checked that everything remained in sync.
…lied Replace `updateDomainModel()` with a simpler function which returns a copy of the input annotation with the changes from the editor applied.
The `updateViewModel()` function was responsible for computing several pieces of data (links, document domain and title) used by the annotation template. Since we can now rely on vm.annotation being immutable, we can just compute this data lazily when we need it and use memoization to avoid unnecessary recalculation.
777e249 to
6f747ba
Compare
Fixed by 6f747ba |
|
Alright, the code changes all look like improvements to me. I've tested this thoroughly and though I found several issues that I could also reproduce on master (and opened GitHub issues for), I only found the one issue that I've GIFted you ( :) ) above that's unique to this branch. There are so many different cases and paths to test with this sidebar that it's hard to comprehensively say that a change doesn't introduce any new bugs. But I think if the one above is fixed then we should merge this. |
The correct fix would be to remove new annotations with an empty draft in the reducer function that handles new annotations being created. That is not currently possible because drafts are not stored in the Redux store. Therefore this commit just manually de-registers the listener when <annotation> goes out of scope.
6f747ba to
8db47b8
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Note: This PR depends on #7
This PR simplifies the annotation component by removing the private state (annotation object, modified tags/text/privacy, editing vs. viewing state) which duplicated data from the annotation it was displaying and the drafts service.
This PR makes the displayed annotation a function of:
vm.annotation)drafts.get(vm.annotation))AnnotationControllerat the moment but will move to the app state in futureAs a consequence of this cleanup, many issues related to drafts re-appearing when switching groups or when clicking Cancel and switching groups have been fixed. Many tests of external <-> private state syncing for the
<annotation>component have now been rendered redundant.I would recommend commit-by-commit review, but testing the whole set together.
Fixes hypothesis/h#3380
Fixes hypothesis/h#3560
Fixes hypothesis/h#3561