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 sidecar service #5920

Merged
merged 21 commits into from
Mar 15, 2024
Merged

Conversation

raintygao
Copy link
Contributor

@raintygao raintygao commented Feb 22, 2024

Description

Support a new layout service called sidecar in dashboard, so that external plugin can use this service as its container. This flyout supports three modes and resizable.

This flyout will only be used in dashboard assistant currently.

Issues Resolved

#5620

Screenshot

image
image
image

Testing the changes

Check List

  • All tests pass
    • yarn test:jest
    • yarn test:jest_integration
  • New functionality includes testing.
  • New functionality has been documented.
  • Update CHANGELOG.md
  • Commits are signed per the DCO using --signoff

Test steps

  1. Open sidecar: You could open sidecar in plugin or other application through core.overlays.sidecar().open, in this method you need to pass MountPoint and sidecar config, like
    { className: 'chatbot-sidecar', config: { dockedMode: 'right', paddingSize: 460 }.
    After that you could see a sidecar container is mounted.
  2. You could drag border button to view resizable effect.
  3. You could change docked Mode or docked padding size through calling setSidecarConfig with new config, like
    { dockedMode: 'takeover', paddingSize: 500 }
  4. You could call hide and show to implement sidecar hide and show effect.

Copy link

codecov bot commented Feb 22, 2024

Codecov Report

Attention: Patch coverage is 91.50943% with 9 lines in your changes are missing coverage. Please review.

Project coverage is 67.21%. Comparing base (31e8481) to head (23223c6).

❗ Current head 23223c6 differs from pull request most recent head 988d1bc. Consider uploading reports for the commit 988d1bc to get more accurate results

Files Patch % Lines
src/core/public/overlays/overlay_service.ts 0.00% 4 Missing ⚠️
...re/public/overlays/sidecar/sidecar_service.mock.ts 62.50% 3 Missing ⚠️
...c/core/public/overlays/sidecar/sidecar_service.tsx 94.28% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5920      +/-   ##
==========================================
+ Coverage   67.20%   67.21%   +0.01%     
==========================================
  Files        3328     3333       +5     
  Lines       64461    64553      +92     
  Branches    10376    10392      +16     
==========================================
+ Hits        43318    43391      +73     
- Misses      18610    18628      +18     
- Partials     2533     2534       +1     
Flag Coverage Δ
Linux_1 31.69% <11.32%> (-0.06%) ⬇️
Linux_2 55.45% <91.26%> (+0.15%) ⬆️
Linux_3 44.65% <11.32%> (-0.13%) ⬇️
Linux_4 34.99% <13.20%> (-0.07%) ⬇️
Windows_1 31.72% <11.32%> (-0.08%) ⬇️
Windows_2 55.41% <91.26%> (+0.15%) ⬆️
Windows_3 44.65% <11.32%> (-0.13%) ⬇️
Windows_4 34.99% <13.20%> (-0.07%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kavilla
Copy link
Member

kavilla commented Feb 23, 2024

Pretty cool.

Will check out the implementation. In the meantime I see there are some snapshots are not updated for tests. Also wanted to check. Is it intentional that the sidecar pushes the elements over when it is mounted?

@raintygao
Copy link
Contributor Author

raintygao commented Feb 23, 2024

Thank you. @kavilla

In the meantime I see there are some snapshots are not updated for tests. Also wanted to check.

I will do some updates to snapshots.

Is it intentional that the sidecar pushes the elements over when it is mounted?

Yes. When plugin calls open function, the sidecar will be mounted with the paddingSize in passed parameter to push elements over.

Copy link
Member

@ashwin-pc ashwin-pc left a comment

Choose a reason for hiding this comment

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

Nice job extending the existing overlays service to expose this feature although i wonder if this is the right approach? This just looks like a resizable flyout to me which can also open from the bottom. If thats what the sidecar is, why cant we just update the Flyout component on OUI? This is a feature that we also need for Discover and potentially in a few other places around the app. By enhancing the flyout in OUI, we avoid needing to declare a new service in core entirely and building a custom resizeable component as you have done here.

cc: @kgcreative @shanilpa

As for the PR itself, I havent reviewed the code yet but had a few high level comments based on the changes:

  1. Can you add tests to the feature? The codecov has highlighted a few good places to do so.
  2. Can you update the readme on how to use this feature. The other overlay services have sections in the readme on how to use them. This will really make this feature a lot easier to use

Copy link
Member

Choose a reason for hiding this comment

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

Can you add some tests for this service similar to the other two overlays?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry the test wasn't updated together. Since this PR needs to be merged before 2.13, so I planed to create PR for code review first. I will add tests later as soon as possible.

src/core/public/overlays/sidecar/rsizable_button.scss Outdated Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, this is a custom component. Why arent we using a component from OUI for this? There is a need to make Flyouts resizeable in discover too. Instead of building a custom resizeable flyout here, we should update the flyout component in OUI to do the same. Even adding the ability to have a flyout that opens from the bottom is needed, so making a change there is the right thing to do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. When I implemented this feature, OUI didn't export this component. I'm not sure whether the date of OUI support is early or later than the target version. so I choose to implement by myself. I think this component can be removed and replaced with a component imported from OUI after OUI supports it.
  2. I think update a resizable container in OUI is reasonable, flyout is only one kind of container, i'm not sure whether we need to support resizable for a specific kind container in OUI.
  3. Flyout currently only supports expansion from right side. I think expanding from the bottom is more like modal rather than adding more direction to flyout.

Copy link
Member

Choose a reason for hiding this comment

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

For what it's worth, I do agree this should be an OUI component. There are a few nuances between flyout and sidecar, but they are mostly visual.

@kgcreative
Copy link
Member

We need the sidecar and a fly out to be able to exist together

@ashwin-pc
Copy link
Member

@kgcreative but cant the two be the same OUI component? They share a lot of features. Dont you think we can combine the two into one interface that is flexible as either? Here are some similarities.

Flyout Sidecar
Lives on document root Lives on document root
Opens from the side opens from sides and bottm
Push and overlay Only push
wants resize Supports resize
... ...

And even of they cant be the same component, the raw component should be bult in OUI and consumed hee like the flyout and Modal. Neither are built in OSD.

@raintygao
Copy link
Contributor Author

raintygao commented Feb 26, 2024

@ashwin-pc
Although sidecar and flyout have some similarities, they are actually totally different. And these two components should be able to exist at the same time without affecting each other.
There are also several technical reasons that this should be implemented in OSD:

  1. The push effect of flyout and sidecar are two completely different approaches. Push of flyout is implemented by adding padding attributes to body DOM, but this can only affect app-wrapper on the OSD and cannot affect the chrome header after testing. However, sidecar needs to push the entire OSD. Due to the structure of the OSD DOM, the component must affect on header and app-wrapper separately to implement the push effect of the entire OSD, which is unable for an external component. Meanwhile I think there are certain risks if rashly modifying the DOM structure of OSD.
  2. There are many customization requirements in the sidecar, such as switching directions in real time without unmounting, the hidden function not only needs to hide the sidecar's DOM but also cancel the push effect of the header and app-wrapper on the OSD. I think component in generic UI library component should not support these custom functions.
  3. Since implementing push effects and resizable involves communication between document root and OSD, observable is used. Performance may not be easy to control if using external components.

I think it's reasonable for OUI to support a single resizable container like some others, but if it involves interacting with entire OSD and some customization requirements, i think this should be handled internally by the OSD, this should be more of a specific component inside the OSD. It may be confused and more difficult to put it as a component in generic UI library.

@ashwin-pc
Copy link
Member

@raintygao Okay i see the difference. I no longer think that the two need to be the same component, but I do still think that the component needs to come from OUI. So my hesitation with the approach is that we are defining a lot of standard UI interaction patterns in OSD for resizing that should ideally be taken care of by OUI.

As for the mount point, I think the way the flyout does it is not ideal but doesnt mean we have to do it the same way. You can always pass a ref to a react component so that it can mount correctly. In this case the app container can be provided to an OUI Sidecar component and it can mount appropriately. All its states and combinations are also easier to test and validate while also abstracting away the accessibility and ui interaction code from the OSD codebase. not sure what your performance concerns are but lets chat about it.

I think component in generic UI library component should not support these custom functions.

Yes. I expects similar separation of concerns similar to how the flyout or modal behaves. The UI Library is only responsible for the UI and resize properties. The state management and what to do once actions are clicked should all be handled by the sidecar service that you have here. It also makes testing these a lot easier since UI tests are quite different from the integ tests that the service needs.

@raintygao
Copy link
Contributor Author

raintygao commented Feb 27, 2024

@ashwin-pc
Thanks. I agree with you that some standard UI components should be imported from OUI, like resizable button, resizable flyouts. Once OUI support these components we can use them to replace raw implementation in OSD sidecar service.

Excluding some reused UI components above, I think the remaining content of sidecar service should be implemented in OSD instead of OUI because it is not a widely used service and currently only used by assistant plugin. Many important parts are handled in this service, such as how chrome header and app-wrapper respond respectively when the container is dragged to implement the overall push effect, some state management, some real-time configuration switching, display and hiding and many customization functions, etc.

@ashwin-pc
Copy link
Member

+1 Can you work on adding these to OUI? Lemme know if you need any help contributing that change :)

@raintygao
Copy link
Contributor Author

raintygao commented Feb 28, 2024

@ashwin-pc
Sure. I don't know if there is a team support new features in OUI, if not, I can work on supporting this. Since I have ever not been working on OUI codebase, it may take some time. My team is rushing on releasing workspace feature on 2.13 so I may not have extra time to support this until 2.14 at the earliest.

Sidecar feature is a high property feature which was implemented in 2.12 and planned to be released in 2.13.
I agree with you that it should be updated. I think what we discussed above is not about design or performance issues, but more like standards for code and component reuse, its priority may should not affect the release and feature delivery, especially my team don't have extra time currently, right?

So my suggestion is that we review and merge current implementation to ensure deliver on time and try to support new UI components in OUI from the next version. When completed, we will replace raw implementation in OSD. I think this is a balanced solution.

@raintygao raintygao dismissed stale reviews from ruanyl and SuZhou-Joe via 23223c6 March 14, 2024 08:29
@raintygao
Copy link
Contributor Author

@SuZhou-Joe @ruanyl I just rebased main to resolve conflicts in changelog, and your approvals were dismissed automatically. Could you approve again?

@SuZhou-Joe SuZhou-Joe merged commit c2c4f53 into opensearch-project:main Mar 15, 2024
55 checks passed
@opensearch-trigger-bot
Copy link
Contributor

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/OpenSearch-Dashboards/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/OpenSearch-Dashboards/backport-2.x
# Create a new branch
git switch --create backport/backport-5920-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 c2c4f53f140a655705e0e8830ea14e8d958a8de9
# Push it to GitHub
git push --set-upstream origin backport/backport-5920-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/OpenSearch-Dashboards/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-5920-to-2.x.

raintygao added a commit to raintygao/OpenSearch-Dashboards that referenced this pull request Mar 15, 2024
* feat: sidecar poc

Signed-off-by: tygao <[email protected]>

* update sidecar

Signed-off-by: tygao <[email protected]>

* update mouseevent

Signed-off-by: tygao <[email protected]>

* update type

Signed-off-by: tygao <[email protected]>

* update service mock

Signed-off-by: tygao <[email protected]>

* update sidecar config mode type

Signed-off-by: tygao <[email protected]>

* update docked mode enum

Signed-off-by: tygao <[email protected]>

* update resizable button style

Signed-off-by: tygao <[email protected]>

* doc: add changelog

Signed-off-by: tygao <[email protected]>

* test: update snapshots

Signed-off-by: tygao <[email protected]>

* fix typo

Signed-off-by: tygao <[email protected]>

* doc: update readme

Signed-off-by: tygao <[email protected]>

* test: add test for resizable button and service

Signed-off-by: tygao <[email protected]>

* test: add test for create react overlays and app container

Signed-off-by: tygao <[email protected]>

* test: add tests for service and components

Signed-off-by: tygao <[email protected]>

* test: update tests for resizable button

Signed-off-by: tygao <[email protected]>

* fix: update enum usage

Signed-off-by: tygao <[email protected]>

* update

Signed-off-by: tygao <[email protected]>

* fix wrong change

Signed-off-by: tygao <[email protected]>

* add prefix for resizableButton

Signed-off-by: tygao <[email protected]>

---------

Signed-off-by: tygao <[email protected]>
(cherry picked from commit c2c4f53)
SuZhou-Joe pushed a commit that referenced this pull request Mar 18, 2024
* feat: sidecar poc

Signed-off-by: tygao <[email protected]>

* update sidecar

Signed-off-by: tygao <[email protected]>

* update mouseevent

Signed-off-by: tygao <[email protected]>

* update type

Signed-off-by: tygao <[email protected]>

* update service mock

Signed-off-by: tygao <[email protected]>

* update sidecar config mode type

Signed-off-by: tygao <[email protected]>

* update docked mode enum

Signed-off-by: tygao <[email protected]>

* update resizable button style

Signed-off-by: tygao <[email protected]>

* doc: add changelog

Signed-off-by: tygao <[email protected]>

* test: update snapshots

Signed-off-by: tygao <[email protected]>

* fix typo

Signed-off-by: tygao <[email protected]>

* doc: update readme

Signed-off-by: tygao <[email protected]>

* test: add test for resizable button and service

Signed-off-by: tygao <[email protected]>

* test: add test for create react overlays and app container

Signed-off-by: tygao <[email protected]>

* test: add tests for service and components

Signed-off-by: tygao <[email protected]>

* test: update tests for resizable button

Signed-off-by: tygao <[email protected]>

* fix: update enum usage

Signed-off-by: tygao <[email protected]>

* update

Signed-off-by: tygao <[email protected]>

* fix wrong change

Signed-off-by: tygao <[email protected]>

* add prefix for resizableButton

Signed-off-by: tygao <[email protected]>

---------

Signed-off-by: tygao <[email protected]>
(cherry picked from commit c2c4f53)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants