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 patch style for oui fixed components #203

Merged
merged 10 commits into from
Jun 7, 2024
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -16,4 +16,5 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
- Add data source service ([#191](https://github.com/opensearch-project/dashboards-assistant/pull/191))
- Update router and controller to support MDS ([#190](https://github.com/opensearch-project/dashboards-assistant/pull/190))
- Hide notebook feature when MDS enabled and remove security dashboard plugin dependency ([#201](https://github.com/opensearch-project/dashboards-assistant/pull/201))
- Refactor default data source retriever ([#197](https://github.com/opensearch-project/dashboards-assistant/pull/197))
- Refactor default data source retriever ([#197](https://github.com/opensearch-project/dashboards-assistant/pull/197))
- Add patch style for fixed components ([#203](https://github.com/opensearch-project/dashboards-assistant/pull/203))
3 changes: 3 additions & 0 deletions public/chat_header_button.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,9 @@ jest.spyOn(coreContextExports, 'useCore').mockReturnValue({
element.style.display = 'block';
}
},
getSidecarConfig$: () => {
return new BehaviorSubject(undefined);
},
};
},
},
Expand Down
2 changes: 2 additions & 0 deletions public/chat_header_button.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import {
} from './utils/constants';
import { useCore } from './contexts/core_context';
import { MountPointPortal } from '../../../src/plugins/opensearch_dashboards_react/public';
import { usePatchFixedStyle } from './hooks/use_patch_fixed_style';

interface HeaderChatButtonProps {
application: ApplicationStart;
Expand Down Expand Up @@ -52,6 +53,7 @@ export const HeaderChatButton = (props: HeaderChatButtonProps) => {
const core = useCore();
const flyoutFullScreen = sidecarDockedMode === SIDECAR_DOCKED_MODE.TAKEOVER;
const flyoutMountPoint = useRef(null);
usePatchFixedStyle();
Copy link
Member

@SuZhou-Joe SuZhou-Joe Jun 7, 2024

Choose a reason for hiding this comment

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

I am thinking, the patched style should apply only when the chatbot is visible right? In current implementation, it will always patch the style to global even if the Chatbot is not mounted, which I think is not clean enough.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Patchstyle contains the logic that when chatbot is closed or docked takeover. When in these cases, we will update a empty style to remove patch style.

Copy link
Member

Choose a reason for hiding this comment

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

I see, so the sidecarConfig$ will emit a null value once the Chatbot is not visible?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sidecar config will emit a new value with isHidden property is true.Then we will update inserted style with ""


useEffectOnce(() => {
const subscription = props.application.currentAppId$.subscribe((id) => setAppId(id));
Expand Down
175 changes: 175 additions & 0 deletions public/hooks/__snapshots__/use_patch_fixed_style.test.ts.snap

Large diffs are not rendered by default.

109 changes: 109 additions & 0 deletions public/hooks/use_patch_fixed_style.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,109 @@
/*
* Copyright OpenSearch Contributors
* SPDX-License-Identifier: Apache-2.0
*/

import { renderHook, act } from '@testing-library/react-hooks';

import { usePatchFixedStyle } from './use_patch_fixed_style';
import * as coreHookExports from '../contexts/core_context';
import { BehaviorSubject } from 'rxjs';
import { ISidecarConfig, SIDECAR_DOCKED_MODE } from '../../../../src/core/public';

describe('usePatchFixedStyle hook', () => {
const sidecarConfig$ = new BehaviorSubject<ISidecarConfig | undefined>({
dockedMode: SIDECAR_DOCKED_MODE.RIGHT,
paddingSize: 300,
});

beforeEach(() => {
jest.spyOn(coreHookExports, 'useCore').mockReturnValue({
overlays: {
// @ts-ignore
sidecar: () => {
return {
getSidecarConfig$: () => {
return sidecarConfig$;
},
};
},
},
});
jest.spyOn(window, 'requestAnimationFrame').mockImplementation((cb) => cb());
});

afterEach(() => {
jest.clearAllMocks();
window.requestAnimationFrame.mockRestore();
});

it('should patch corresponding left style when sidecarConfig$ pipe', async () => {
renderHook(() => usePatchFixedStyle());
act(() =>
sidecarConfig$.next({
dockedMode: SIDECAR_DOCKED_MODE.LEFT,
paddingSize: 300,
})
);

expect(document.head).toMatchSnapshot();
});

it('should patch corresponding right style when sidecarConfig$ pipe', async () => {
renderHook(() => usePatchFixedStyle());
act(() =>
sidecarConfig$.next({
dockedMode: SIDECAR_DOCKED_MODE.RIGHT,
paddingSize: 300,
})
);

expect(document.head).toMatchSnapshot();
});

it('should patch empty style when isHidden of sidecarConfig$ is true', async () => {
renderHook(() => usePatchFixedStyle());
act(() =>
sidecarConfig$.next({
dockedMode: SIDECAR_DOCKED_MODE.LEFT,
paddingSize: 300,
isHidden: true,
})
);

expect(document.head).toMatchSnapshot();
});

it('should patch empty style when dockedMode of sidecarConfig$ is takeover', async () => {
renderHook(() => usePatchFixedStyle());
act(() =>
sidecarConfig$.next({
dockedMode: SIDECAR_DOCKED_MODE.TAKEOVER,
paddingSize: 300,
})
);

expect(document.head).toMatchSnapshot();
});

it('should not subscribe update after unmount', async () => {
const { unmount } = renderHook(() => usePatchFixedStyle());
act(() =>
sidecarConfig$.next({
dockedMode: SIDECAR_DOCKED_MODE.RIGHT,
paddingSize: 300,
})
);
expect(document.head).toMatchSnapshot();

unmount();

act(() =>
sidecarConfig$.next({
dockedMode: SIDECAR_DOCKED_MODE.LEFT,
paddingSize: 500,
})
);
expect(document.head).toMatchSnapshot();
});
});
88 changes: 88 additions & 0 deletions public/hooks/use_patch_fixed_style.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
/*
* Copyright OpenSearch Contributors
* SPDX-License-Identifier: Apache-2.0
*/
import { useEffect, useRef } from 'react';

import { useCore } from '../contexts/core_context';
import { ISidecarConfig, SIDECAR_DOCKED_MODE } from '../../../../src/core/public';

// There are some UI components from library whose position is fixed and are not compatible with each other and also not compatible with sidecar container.
// There is currently no way to provide config for these components at runtime.
// This hook patches a style for all these already known components to make them compatible with sidecar.
// TODO: Use config provider from UI library to make this more reasonable.
export const usePatchFixedStyle = () => {
const core = useCore();
const sidecarConfig$ = core.overlays.sidecar().getSidecarConfig$();

useEffect(() => {
const css = '';
const style = document.createElement('style');
const text = document.createTextNode(css);
document.head.appendChild(style);
style.appendChild(text);

const subscription = sidecarConfig$.subscribe((config) => {
if (!config) return;
updateHeadStyle(config, text);
});
return () => {
subscription.unsubscribe();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shall we remove appended style after unmount?

document.head.removeChild(style);
style.removeChild(text);
};
}, [sidecarConfig$]);
};

function updateHeadStyle(config: ISidecarConfig, text?: Text) {
let css = '';
if (!text) return;

if (
// When sidecar is opened and docked position is left or right, we should patch style.
config?.isHidden !== true &&
config.paddingSize &&
(config.dockedMode === SIDECAR_DOCKED_MODE.LEFT ||
config.dockedMode === SIDECAR_DOCKED_MODE.RIGHT)
) {
const { dockedMode, paddingSize } = config;
if (dockedMode === SIDECAR_DOCKED_MODE.RIGHT) {
// Current applied components include flyout and bottomBar.
// Although the class names of actual rendered component start with eui. We will also apply oui for fallback.
css = `
.euiFlyout:not(.euiFlyout--left) {
right: ${paddingSize}px
}
.ouiFlyout:not(.ouiFlyout--left) {
right: ${paddingSize}px
}
.euiBottomBar{
padding-right: ${paddingSize}px
}
.ouiBottomBar{
padding-right: ${paddingSize}px
}
`;
} else if (dockedMode === SIDECAR_DOCKED_MODE.LEFT) {
css = `
.euiFlyout--left {
left: ${paddingSize}px
}
.ouiFlyout--left {
left: ${paddingSize}px
}
.euiBottomBar{
padding-left: ${paddingSize}px
}
.ouiBottomBar{
padding-left: ${paddingSize}px
}
`;
}
}

// If sidecar closes or docked direction change to takeover mode, we will keep css empty to remove patch style and update.
requestAnimationFrame(() => {
text.textContent = css;
});
Comment on lines +87 to +89
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
requestAnimationFrame(() => {
text.textContent = css;
});
requestAnimationFrame(() => {
if (config?.isHidden) { text.textContent = ''; return ; }
text.textContent = css;
});

Nit: I'd suggest explicitly making the textContent as empty.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think here takeover dockedMode also takes the same effect as isHidden, should we also explicitly append?

}
Loading