uses image component to add graphics to annotation

[jasenlo123]

• Please check if the PR fulfills these requirements

• Tests for the changes have been added (for bug fixes / features)
• Docs have been added / updated (for bug fixes / features)

• What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
  Uses the Image component to for displaying images inside of the annotation div.

• What is the current behavior? (You can also link to an open issue here)
  Current Annotation component does not allow for images.

• What is the new behavior (if this is a feature change)?
  New Annotation component places images at the start of the annotation, can have text and image in the same annotation div.

• Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
  None

• Other information:

[mathisonian]

Thanks @jasenlo123!

I have been thinking about this, and wanted to get your thoughts on possible API to make it slightly more flexible. What if we do something like:

[Annotation] 
   [Graphic] Put your annotation content here. [/Graphic]
   The text. 
[/Annotation]

or with an image:

[Annotation] 
   [Graphic] [Image src:"..." /] [/Graphic]
   The text. 
[/Annotation]

I think this would be nice because it matches the existing APIs of the Scrollers and Steppers of using Graphic to hold a referent element, and it is more flexible in that you can put in arbitrary components in the annotation, rather than just an image or text.

[mathisonian]

To see how to implement this, the stepper component is a good example, e.g. to place the graphic in the render tree

[jasenlo123]

helu @mathisonian!

Oh this implementation makes so much more sense. I was bothered by how inflexible my implementation was. I'll look into your suggestion and update my PR soon.

[mathisonian]

Great! Sounds good @jasenlo123

[jasenlo123]

Hi @mathisonian,

Did as you suggested by matching the Annotation implementation with the existing APIs of Scroller and Stepper of using Graphic to hold a referent element. The Idyll Markdown for the examples in the screenshots below are in this gist!

• Example with Chart as a referent element.

• Example with Image as a referent element.

Note that now the annotation prop refers to the inline text, whereas the children are any graphics or text. This is contrary to the existing implementation, where the annotation prop referred to the annotation box text while the children were the inline text.

NOW:

[Annotation annotation:" This is the highlighted inline text."] 
   [Graphic] [Image src:"..." /] [/Graphic]
   This text is the stuff in the annotation box.
[/Annotation]

EXISTING

[Annotation annotation:" This text is the stuff in the annotation box."] 
   This is the highlighted inline text.
[/Annotation]

I'll spend some time testing the component with different elements in the Graphic to see if anything breaks, but so far this seems to work okie. Curious to hear your thoughts + feedback!

[mathisonian]

These updates look great @jasenlo123! I think this approach will give a much more powerful + flexible component.

Sorry I haven't had a chance to do a deep dive and give feedback yet but will do it soon.

[mathisonian]

@jasenlo123 this looks great! I'd be happy to merge as-is, but want to get your thoughts on one potential alternative API:

Since [Annotation annotation:""]... is a little redundant, we might make it more explicit by changing to [Annotation text:""]... or by changing it to something like:

currently in the PR:

[Annotation annotation:" This is the highlighted inline text."] 
   [Graphic] [Image src:"..." /] [/Graphic]
   This text is the stuff in the annotation box.
[/Annotation]

becomes:

[Annotation] 
   [Graphic] 
      [Image src:"..." /] 
      This text is the stuff in the annotation box.
   [/Graphic]

   This is the highlighted inline text.
[/Annotation]

[jasenlo123]

Hi @mathisonian!

I implemented your second suggestion:

[Annotation] 
   [Graphic] 
      [Image src:"..." /] 
      This text is the stuff in the annotation box.
   [/Graphic]

   This is the highlighted inline text.
[/Annotation]

I do agree that it is easier to work with. I'm having some trouble with tests though, despite it working okay. I use slice() in my annotation.js, which is throwing a type error when I run yarn test: TypeError: Cannot read property 'slice' of undefined. I'm assuming the error has something to do with the test for Annotation having no children and thus having no array to slice? Thoughts?

Another thing that is bothering me is that there seems to be a space that is part of the inline annotation text that is highlighted when I use the annotation. This is some example markdown for the suggested implementation, and image to draw attention to the problem:

[mathisonian]

Thanks for the patches @jasenlo123! I'll take a look and push some fixes for the two issues that you mentioned, then I think we should be good to merge 👍

[mathisonian]

@jasenlo123 I pushed a few edits to address the issues you mentioned above (not erring out on slice) and updated the markup so that there will always be a space between the annotation text and other text that precedes or follows it.

The padding inside the gray box that you mentioned is coming from a CSS rule: .annotated-text has padding set to 0 5px. IMO this is okay but we can reduce if it looks weird to you

[jasenlo123]

@mathisonian

facepalm on the padding: 0 5px. I changed it to padding: 0 2.5px, I can see how one might be frustrated with figuring out their Idyll markdown to find that pesky imaginary space that bothered me.

Thanks for fixing up the my slice() error - shoddy JS is shoddy. Everything looks good to me and ready to merge!

[mathisonian]

Sounds good to me! Thanks again @jasenlo123. This will be available in the next release (probably next week by the time I have a chance to update all the docs)