Skip to content

feat(annotations): update url on annotation selection#2079

Merged
mickr merged 13 commits intobox:masterfrom
mickr:add-handler-for-annotation-activity-card
Apr 17, 2020
Merged

feat(annotations): update url on annotation selection#2079
mickr merged 13 commits intobox:masterfrom
mickr:add-handler-for-annotation-activity-card

Conversation

@mickr
Copy link
Contributor

@mickr mickr commented Apr 14, 2020

Adding handler to update URL from annotation activity card, and call the callback from contentPreview to handle selecting annotation in Preview

  • Tests

@ConradJChan
Copy link
Contributor

Few thoughts on this:

  • What are your thoughts on instead of handleAnnotationSelect is that we could put a function on the AnnotatorContext similar to how we do connected components in redux:
export interface AnnotatorState {
    activeAnnotationId?: string | null;
    annotation?: object;
    action?: Action;
    error?: Error;
    setActiveAnnotationId: (id: string) => void;
}

In this way, we can avoid the onAnnotationSelect passing down through the component tree.

  • For the deep-linking to the annotation item, we need to handle it being initiated by either box-annotations or clicking the link in the annotation item -- what do you think about only relying on the box-annotations annotations_active_change event to trigger a history.push|replace to simplify the flow?

@mickr mickr marked this pull request as ready for review April 16, 2020 16:29
@mickr mickr requested review from a team as code owners April 16, 2020 16:29
const { history } = this.props;

const urlPrefix = this.getUrlPrefix(history.location.pathname);
history.replace(`/${urlPrefix}/annotations/${annotationId}`);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we want to push instead of replace here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this URL going to suffice for our purposes? Will we be able to determine the version we need to load?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, but I talked with the BE team and they have not worked through the versions url yet and what it will look like.

mickr added 11 commits April 16, 2020 14:03
* Add history replace to update url when clicking annotation activity item
* Add proper activeFeedEntryType for focus
* Fix focus state for activity item
* pass callback function to sidebar to be called on annotation selection
* Fix flow errors
* Add message for missing annotation
* Adding handler to annotationContext
* Update tests for withAnnotations
* Re-add an instance of onAnnotationSelect
* Add annotator to withAnnotations context to emit back to annotations.
@box box deleted a comment Apr 17, 2020

export interface AnnotatorContext {
state: AnnotatorState;
emitActiveChangeEvent: (id: string) => void;
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about naming this as something more like setActiveAnnotationId instead to abstract away the implementation details of how we set the active annotation id?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it is more descriptive with this name, not sure if this detail needs to be abstracted away.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok then at least make it emitActiveSetEvent?

>
<WrappedComponent
{...this.props}
onAnnotator={this.handleOnAnnotator}
Copy link
Collaborator

Choose a reason for hiding this comment

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

handleAnnotator instead of handleOnAnnotator

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can follow up on this

* Add the annotator emit handler to context
* Move annotatorState to state object
* Updated tests
@mickr mickr changed the title Add handler for annotation activity card feat(annotations): update url on annotation selection Apr 17, 2020
@mickr mickr merged commit 98434cd into box:master Apr 17, 2020
@mickr mickr deleted the add-handler-for-annotation-activity-card branch April 17, 2020 20:47
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