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

Rename the "$$tag" annotation property and remove Object.assign() hacks #126

Merged
merged 2 commits into from
Oct 24, 2016

Conversation

robertknight
Copy link
Member

The "$$tag" annotation property which stores the local ID of annotations had a "$$" prefix for historical reasons and had to be explicitly copied when shallow-cloning annotation objects with Object.assign() because it was non-enumerable.

Following #116 , this can now be cleaned up within the sidebar app. This PR renames $$tag to $tag to match the other local-only annotation properties and removes the Object.assign() hacks.

@sean-roberts
Copy link
Contributor

LGTM. However, lot's of code change here and some historical context, it should probably be checked by someone how knows what else to expect from this change

@nickstenning
Copy link
Contributor

Needs a rebase.

@nickstenning
Copy link
Contributor

Let's get this rebased so we can get it merged?

This property previously had a "$$" prefix instead of the conventional
single "$" prefix for local-only properties of annotations for
historical reasons.
@sheetaluk
Copy link
Contributor

LGTM. Tested it out on http://localhost:5000/docs/help by creating, editing, replying to and deleting an annotation and seems to work fine.
Note: Replacing $$tag with $tag in the annotator is not part of this PR.

@sheetaluk sheetaluk merged commit 252728c into master Oct 24, 2016
@sheetaluk sheetaluk deleted the rename-local-tag branch October 24, 2016 14:11
sheetaluk pushed a commit that referenced this pull request Oct 24, 2016
This property previously had a "$$" prefix instead of the conventional
single "$" prefix for local-only properties of annotations for
historical reasons.

This is a followup PR to:
#126
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.

4 participants