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

Fix: Only show create dialog when creating new point annotations #3

Closed
wants to merge 7 commits into from
Closed

Conversation

pramodsum
Copy link
Contributor

@pramodsum pramodsum commented Oct 12, 2017

iphone
tablet

@@ -231,6 +239,16 @@ class Annotator extends EventEmitter {
</div>`.trim();

this.container.appendChild(mobileDialogEl);

if (this.isModeAnnotatable(TYPES.point)) {
this.createPointDialog = new CreateAnnotationDialog(this.container, {
Copy link
Contributor

Choose a reason for hiding this comment

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

naming?

* Events emitted by this component.
* TODO(@spramod): Evaluate if these events need to be propogated to viewer
*/
export const CreateEvents = {
Copy link
Contributor

Choose a reason for hiding this comment

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

All caps?

@@ -0,0 +1,270 @@
import EventEmitter from 'events';
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this fit in with our future model of using modes/controllers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This fits in with that, because this is a shared point annotation creation dialog. Right now this is just for desktop but ideally we would be just triggering this dialog w/o creating a thread for both mobile and desktop.

Copy link
Contributor

@JustinHoldstock JustinHoldstock left a comment

Choose a reason for hiding this comment

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

Awesome cleanup

/** @property {HTMLElement} - The parent container to nest the dialog element in. */
parentEl;

/** @property {HTMLElement} - The element containing the buttons that can creaet highlights. */
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo!

creaet -> create

@@ -0,0 +1,270 @@
import EventEmitter from 'events';
Copy link
Contributor

Choose a reason for hiding this comment

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

Great stuff!

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.

3 participants