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

Annotation classes #57

Merged
merged 23 commits into from
Oct 22, 2018
Merged

Annotation classes #57

merged 23 commits into from
Oct 22, 2018

Conversation

tim-evans
Copy link
Collaborator

@tim-evans tim-evans commented Jun 25, 2018

This branch is a culmination of a deep rewrite of AtJSON, making Annotations classes instead of object literals.

Previously, annotations were ephemeral, and were loosely defined in sources and renderers. With changes made here, annotations will have an explicit class where they can be documented, altered, and define extension points.

Bold annotations are one of the most basic for text editing. Creating a bold annotation looked like:

doc.addAnnotation({
   type: 'bold',
   start: 0,
   end: 5
});

Now, adding a bold annotation is done using a class + a vendor prefixed version of it. To add support for Bold annotations in our rich text editor, Offset, the following class would be made:

import { InlineAnnotation } from '@atjson/document';

class Bold extends InlineAnnotation {
  static vendorPrefix = 'offset';
  static type = 'bold';
}

And we would add it to our document by adding the following JSON:

doc.addAnnotation({
  type: '-offset-bold',
  start: 0,
  end: 5,
  attributes: {}
});

Note that the vendor prefix is prepended (like a CSS vendor prefix) to the beginning of the type of the annotation. This is to help to be clear with distinguishing between various types of annotations that exist on multiple sources, and stop them from clobbering one another. In addition, attributes is now a required field on the Annotation JSON.

@tim-evans tim-evans added the 🏭 In Progress This pull request is in progress~ be patient while it gets done label Jun 25, 2018
@tim-evans tim-evans added the 🚨 Breaking Change This introduces a breaking change to atjson. label Jun 26, 2018
Copy link
Contributor

@blaine blaine left a comment

Choose a reason for hiding this comment

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

Sooooorry for the massive delay on reviewing this - tbh, I thought I'd already approved, but clearly not. 🙃 😢

This is great, and looking forward to moving the change behaviour to Annotations so that we can move some of the conditional logic that's creeping into Document & offset out to Annotations.

* In the future, we want to return a set of changes that
* are applied to the document including annotations.
*/
handleChange(change: Change) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it make sense to also abstract adding/removing/changing annotations as Changes? High-level, that feels the right approach, but it might be impractical at the moment.

@foobarrio
Copy link
Contributor

The JS objects in the opening line is tripping me up. How do you feel about something more along the lines of:

"This branch is a culmination of a deep rewrite of AtJSON to represent Annotation objects using class instances instead of object literals."


export { Annotation, Schema, Display };
Copy link
Contributor

Choose a reason for hiding this comment

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

A few places still try to import Display; want I should make a PR against this to fix?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@foobarrio thanks, but it's ok for now! there's still a bunch more to be done with this rewrite. If you'd like to help, I am happy to have you bring you up to speed and help you contribute ❤️

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oooh! I think you could help create our base annotation package! That would be very helpful :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I tried to tsc build and it balks on being unable to import Display is all.

Happy to help w/ whatever--point me in the right direction!

This casts any annotations not identified in the schema list into an UnknownAnnotation. Unknown Annotations are AtJSON's form of Ruby's `method_missing`, with the added benefit that these annotations will be stored and updated in a document even after being updated.
🚨 Render hooks now take annotations instead of props
This defines a subset of the HTML spec as annotations. In the future, we'd like these to be automatically generated from the HTML spec.
@tim-evans tim-evans removed the 🏭 In Progress This pull request is in progress~ be patient while it gets done label Oct 22, 2018
@tim-evans tim-evans merged commit a1b83f0 into latest Oct 22, 2018
@foobarrio foobarrio deleted the annotation-classes branch October 23, 2018 14:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🚨 Breaking Change This introduces a breaking change to atjson.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants