Skip to content

Commit

Permalink
Fix sidebar container initialization when "clean" theme is used
Browse files Browse the repository at this point in the history
When the "clean" theme is used, the sidebar's toggle button is not created and
constructing the `DragHandler` failed because it was passed a `null` target.
Update the types in `ToolbarController.sidebarToggleButton` to reflect that the
value can be null and handle this in `Sidebar` by not initializing
`_dragResizeHandler` in this case.
  • Loading branch information
robertknight committed Nov 6, 2024
1 parent 39cf23d commit e15e597
Show file tree
Hide file tree
Showing 3 changed files with 30 additions and 12 deletions.
17 changes: 11 additions & 6 deletions src/annotator/sidebar.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ type DragResizeState = {
export class Sidebar implements Destroyable {
private _emitter: Emitter;
private _config: SidebarContainerConfig & SidebarConfig;
private _dragResizeHandler: DragHandler;
private _dragResizeHandler: DragHandler | undefined;
private _dragResizeState: DragResizeState;
private _listeners: ListenerCollection;
private _layoutState: SidebarLayout;
Expand Down Expand Up @@ -292,10 +292,15 @@ export class Sidebar implements Destroyable {
initial: null,
final: null,
};
this._dragResizeHandler = new DragHandler({
target: this.toolbar.sidebarToggleButton,
onDrag: event => this._onDragSidebarToggleButton(event),
});

const toggleButton = this.toolbar.sidebarToggleButton;
if (toggleButton) {
this._dragResizeHandler = new DragHandler({
target: toggleButton,
onDrag: event => this._onDragSidebarToggleButton(event),
});
}

this.close();

// Publisher-provided callback functions
Expand Down Expand Up @@ -327,7 +332,7 @@ export class Sidebar implements Destroyable {
this._sidebarRPC.destroy();
this.bucketBar?.destroy();
this._listeners.removeAll();
this._dragResizeHandler.destroy();
this._dragResizeHandler?.destroy();
if (this._hypothesisSidebar) {
// Explicitly unmounting the "messages" element, to make sure effects are clean-up
render(null, this._messagesElement!);
Expand Down
15 changes: 13 additions & 2 deletions src/annotator/test/sidebar-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -154,15 +154,26 @@ describe('Sidebar', () => {
fakeDragHandler = {
destroy: sinon.stub(),
};
FakeDragHandler = sinon.stub().returns(fakeDragHandler);
FakeDragHandler = sinon.stub().callsFake(options => {
assert.isNotNull(options.target);
assert.isFunction(options.onDrag);
return fakeDragHandler;
});

const toggleButton = document.createElement('button');
fakeToolbar = {
getWidth: sinon.stub().returns(100),
useMinimalControls: false,
sidebarOpen: false,
newAnnotationType: 'note',
highlightsVisible: false,
sidebarToggleButton: document.createElement('button'),
get sidebarToggleButton() {
if (this.useMinimalControls) {
return null;
} else {
return toggleButton;
}
},
};
FakeToolbarController = sinon.stub().returns(fakeToolbar);

Expand Down
10 changes: 6 additions & 4 deletions src/annotator/toolbar.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ export class ToolbarController {
private _toggleSidebar: () => void;
private _toggleHighlights: () => void;
private _createAnnotation: () => void;
private _sidebarToggleButton: RefObject<HTMLElement>;
private _sidebarToggleButton: RefObject<HTMLButtonElement>;

/**
* @param container - Element into which the toolbar is rendered
Expand All @@ -52,7 +52,7 @@ export class ToolbarController {
};

/** Reference to the sidebar toggle button. */
this._sidebarToggleButton = createRef<HTMLElement>();
this._sidebarToggleButton = createRef();

this.render();
}
Expand Down Expand Up @@ -115,9 +115,11 @@ export class ToolbarController {

/**
* Return the DOM element that toggles the sidebar's visibility.
*
* This will be `null` if {@link useMinimalControls} is true.
*/
get sidebarToggleButton() {
return this._sidebarToggleButton.current as HTMLButtonElement;
get sidebarToggleButton(): HTMLButtonElement | null {
return this._sidebarToggleButton.current;
}

render() {
Expand Down

0 comments on commit e15e597

Please sign in to comment.