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

Add Annotation Feature #1858

Merged
merged 78 commits into from Apr 20, 2022
Merged

Add Annotation Feature #1858

merged 78 commits into from Apr 20, 2022

Conversation

ong6
Copy link
Contributor

@ong6 ong6 commented Mar 27, 2022

What is the purpose of this pull request?

  • Documentation update
  • Bug fix
  • Feature addition or enhancement
  • Code maintenance
  • Others, please explain:

Fixes #898

Overview of changes:

*Note that the implementation has changed completely after discussion with @damithc

Added basic annotation feature into markbind with the idea of allowing users to add popovers and text to their existing images. Now works by making use of the existing floating-vue lib to help manage popovers over the image. Users will now specify the image to annotate then add child <a-points> to draw circles over the image which can function as markers for annotations.

Example Code:

<annotate src="https://www.researchgate.net/profile/Chaiwat-Sakul/publication/228949121/figure/fig2/AS:300654661783574@1448693065750/The-previous-square-root-circuit-1.png" alt="pic" eager type="point" width="500" >
  <a-point x="25%" y="30%" body="Lorem ipsum dolor sit amet" />
  <a-point x="50%" y="50%" label="1" size="50" header="Lorem ipsum"  body="Lorem ipsum dolor sit amet" legend="bottom" color="blue" opacity="0.1" size="100" />
</annotate>

Example Output:
image

The first <a-point> is the minimal template where an x, y and text body has to be entered. The second <a-point> showcases some of the changes users can make to the annotation.

Anything you'd like to highlight / discuss:

<annotate> functions very similarly to <pic> but after discussing with @damithc we decided to split up the 2 classes to enable easier extension in the future. (E.g. adding the arrow implementation etc...)

Future improvements can be tracked at this issue #1911

Shoutout to @jovyntls for helping me with the reactivity of the points!

Testing instructions:
Visit the Netlify Preview and head to annotation under images to test out the feature

or

  1. inside a test markbind website run markbind serve -d
  2. Copy the code snippet above and see if it works.
  3. Play around with some of the other features

Proposed commit message: (wrap lines at 72 characters)

Adding a new Annotation Feature

Users want to annotate over images but have no way of doing so.

Implementing a <annotate> and <a-point> components to help users annotate over images.

This allows users to specify the image in <annotate> and specify the points in <a-point>.

Refer to issue #898 for initial suggestion.


Checklist: ☑️

  • Updated the documentation for feature additions and enhancements
  • Added tests for bug fixes or features
  • Linked all related issues
  • No unrelated changes

@damithc
Copy link
Contributor

damithc commented Mar 27, 2022

Good work @ong6
Note that for this feature to be really useful (and different from simply editing the image to add text), the annotations should be dynamic and non intrusive (i.e., should not obscure/block the original image content), somewhat like popovers.

@ong6
Copy link
Contributor Author

ong6 commented Mar 27, 2022

Good work @ong6 Note that for this feature to be really useful (and different from simply editing the image to add text), the annotations should be dynamic and non intrusive (i.e., should not obscure/block the original image content), somewhat like popovers.

Ah ok, that will have to be worked on as a future milestone as well 😅. Might need to research more on popovers. Issue is that my current implementation draws the whole image initially, so the lines are part of the image as well. I'll look into alternative implementations that might fix this issue though.

@tlylt
Copy link
Contributor

tlylt commented Mar 28, 2022

Good progress @ong6

Some initial comments for the PR (since it's still a work in progress and has quite a bit of code that will probably be removed afterward:

  • Just to be consistent, perhaps the tags can all be lowercase e.g. <annotate> instead of <Annotate>
  • Besides specifying the color of the arrow, perhaps can consider the direction as well. I feel that it's more natural if the arrow is pointing towards the text instead.
  • Looking at the implementation of the text and its styling, it seems pretty limited because of the choice to draw it via canvas. Perhaps we can explore an alternative so that future improvements on this can be easier. Not too sure if this is relevant, but I saw this chartjs annotate plugin that seems to be doing annotation over the canvas as well, perhaps can take a look at its implementation for inspiration.

@jovyntls
Copy link
Contributor

Thank you @ong6 , this looks like a really challenging and interesting issue!

Some other ideas/notes mostly on syntax:

  • annotate-wrapper vs annotatewrapper: Personally I prefer kebab-case since "Annotate Wrapper" can be quite a long phrase, but I've noticed that Markbind components seem to use a mix of kebab-case (e.g. page-nav, page-nav-button, site-nav-button) and lowercase (e.g. navbar, searchbar)
  • On supporting markdown styles: I couldn't find any solution that supports parsing markdown with HTML canvas. I'm not sure if we should manually parse the rest of markdown syntax, since there may be many edge cases. If only headers are supported, providing a header attribute could be another option.

More feature suggestions:

  • Specifying the starting point of the arrow using percentages, e.g. x="50%" y="50%" refers to the centre of the image

@damithc
Copy link
Contributor

damithc commented Mar 28, 2022

@ong6 given below is what I mentioned earlier today, as an alternative to arrows and text inside the image. The text can be just regular text (below) the image. Doesn't even need to be part of annotations as we can use regular MarkBind for it.

image

  1. some explanation
  2. some other explanation

Copy link
Contributor

@jonahtanjz jonahtanjz left a comment

Choose a reason for hiding this comment

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

Thanks @ong6!

Just 3 more nits. The rest looks good :)

docs/userGuide/syntax/annotations.md Outdated Show resolved Hide resolved
docs/userGuide/syntax/annotations.md Outdated Show resolved Hide resolved
docs/userGuide/syntax/annotations.md Outdated Show resolved Hide resolved
ong6 and others added 4 commits April 19, 2022 14:45
Co-authored-by: Jonah Tan <47470981+jonahtanjz@users.noreply.github.com>
Co-authored-by: Jonah Tan <47470981+jonahtanjz@users.noreply.github.com>
Co-authored-by: Jonah Tan <47470981+jonahtanjz@users.noreply.github.com>
Copy link
Contributor

@jonahtanjz jonahtanjz left a comment

Choose a reason for hiding this comment

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

Just noticed this

The images over at Full syntax reference and Syntax cheat sheet are broken.

image

image

@ong6
Copy link
Contributor Author

ong6 commented Apr 19, 2022

Just noticed this

The images over at Full syntax reference and Syntax cheat sheet are broken.

image

image

Nice spot, I just pushed a fix for it

Copy link
Contributor

@jonahtanjz jonahtanjz left a comment

Choose a reason for hiding this comment

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

Thanks @ong6!

Nice work on this new feature 👍

LGTM from my end :)

@jonahtanjz jonahtanjz added this to the 4.0 milestone Apr 19, 2022
@ong6 ong6 requested review from jovyntls and tlylt April 19, 2022 10:38
Copy link
Contributor

@tlylt tlylt left a comment

Choose a reason for hiding this comment

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

Some minor nits from my end, otherwise looking good!

docs/userGuide/syntax/annotations.md Outdated Show resolved Hide resolved
docs/userGuide/syntax/annotations.md Outdated Show resolved Hide resolved
docs/userGuide/syntax/annotations.md Outdated Show resolved Hide resolved
docs/userGuide/syntax/annotations.md Show resolved Hide resolved
ong6 and others added 6 commits April 19, 2022 20:43
Co-authored-by: Liu YongLiang <41845017+tlylt@users.noreply.github.com>
Co-authored-by: Liu YongLiang <41845017+tlylt@users.noreply.github.com>
Co-authored-by: Liu YongLiang <41845017+tlylt@users.noreply.github.com>
Copy link
Contributor

@ryoarmanda ryoarmanda left a comment

Choose a reason for hiding this comment

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

Nice work! Looks solid and clean, kudos 👍 LGTM

Copy link
Contributor

@jovyntls jovyntls left a comment

Choose a reason for hiding this comment

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

LGTM 🎉 🎉 🎉 Thanks for your patience in making the changes @ong6 !

@tlylt tlylt merged commit a595d05 into MarkBind:master Apr 20, 2022
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.

Support image annotation
6 participants