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

feat: Introduce registerCollapsibleNavHeader to ChromeService #5244

Conversation

ruanyl
Copy link
Member

@ruanyl ruanyl commented Oct 7, 2023

Description

This commit introduces an enhancement to the ChromeService class within our core application. It adds a new method named registerCollapsibleNavHeader, allowing plugins to customize the rendering of the collapsible navigation header in the global chrome UI.

With this new capability, plugins can now register their own rendering logic for the collapsible navigation header. This feature enhances the extensibility of our core system, empowering plugins to provide a more tailored and user-friendly navigation experience.

Key changes in this commit include:

  1. The addition of the registerCollapsibleNavHeader method to the ChromeService class.
  2. Appropriate updates to tests and typings to support the newly introduced functionality.

Why introducing this change?

The upcoming workspace feature will bring noticeable change to the left navigation menu. One newly added component is the "Workspace Selector" component. This is a context menu on the top of the left navigation bar which enables easy navigation among workspaces. This PR tries to extend the chrome module with a generic method for plugins to customize the navigation bar's header.

I'm more seeking for suggestions/different opinions with this PR, please feel free to give feedback if you have different implementation ideas.

image

Issues Resolved

Partially resolved: #5091

Screenshot

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

@codecov
Copy link

codecov bot commented Oct 7, 2023

Codecov Report

Attention: Patch coverage is 87.50000% with 1 line in your changes missing coverage. Please review.

Project coverage is 67.02%. Comparing base (d8cbc17) to head (e2fc4d6).
Report is 690 commits behind head on main.

Files with missing lines Patch % Lines
...c/core/public/chrome/ui/header/collapsible_nav.tsx 0.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #5244   +/-   ##
=======================================
  Coverage   67.02%   67.02%           
=======================================
  Files        3294     3294           
  Lines       63298    63306    +8     
  Branches    10067    10069    +2     
=======================================
+ Hits        42424    42431    +7     
  Misses      18431    18431           
- Partials     2443     2444    +1     
Flag Coverage Δ
Linux_1 35.24% <12.50%> (-0.01%) ⬇️
Linux_2 55.19% <87.50%> (+0.01%) ⬆️
Linux_3 43.86% <12.50%> (-0.01%) ⬇️
Linux_4 35.34% <12.50%> (-0.01%) ⬇️
Windows_1 35.26% <12.50%> (-0.01%) ⬇️
Windows_2 55.15% <87.50%> (+0.01%) ⬆️
Windows_3 43.88% <12.50%> (-0.01%) ⬇️
Windows_4 35.34% <12.50%> (-0.01%) ⬇️

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.

@ashwin-pc
Copy link
Member

ashwin-pc commented Oct 11, 2023

@ruanyl This approach seems reasonable to me. Two other alternatives you can look at here are:

  1. Following the observable pattern that the rest of the elements in the panel follow. I dont like this because the nav header is not a dynamically updating component, so that feels unnecessary.
  2. Making the API not so generic and bringing core parts of workspace into the core application. I like this but this makes workspaces a dependency for the core system.

Given the draw backs with both the other approaches, I think your solution here makes the most sense. The one drawback with your API though is that another plugin can override your registration.

@ruanyl
Copy link
Member Author

ruanyl commented Oct 12, 2023

@ashwin-pc Thanks for the feedbacks! About this comment:

The one drawback with your API though is that another plugin can override your registration.

You're right, I'm actually aware of this. There could potentially two options:

  1. check the existence of the render, and throw error/warning if someone tries to override it
  2. make the header to accept multiple customer renders

However, I'm not sure which one will best suite the real use case, to avoid over-optimize at the early stage, I keep it like this for now as currently there is only one use case from workspace plugin. We can always come back to this later.

@ruanyl ruanyl force-pushed the feature/chrome-service-support-custom-menu-header branch from cf82469 to 92d1251 Compare October 12, 2023 10:03
@ashwin-pc
Copy link
Member

Good to know that you thought of it :) Yeah i like your first option to add a check during registration to throw an error if registration is attempted twice. This way we can at least know when this happens.

Copy link
Member

@zengyan-amazon zengyan-amazon left a comment

Choose a reason for hiding this comment

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

LGTM

@ruanyl ruanyl force-pushed the feature/chrome-service-support-custom-menu-header branch from ae21637 to de14bf0 Compare October 18, 2023 06:44
@ruanyl
Copy link
Member Author

ruanyl commented Oct 18, 2023

@ashwin-pc I added console.warn output when the header render was overridden, let me know if that looks good to you :)

ashwin-pc
ashwin-pc previously approved these changes Dec 13, 2023
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.

LGTM, @ruanyl can you resolve the conflicts in the changelog ?

This commit introduces an enhancement to the ChromeService class
within our core application. It adds a new method named
`registerCollapsibleNavHeader`, allowing plugins to customize the
rendering of the collapsible navigation header in the global chrome UI.

With this new capability, plugins can now register their own
rendering logic for the collapsible navigation header. This feature
enhances the extensibility of our core system, empowering plugins to
provide a more tailored and user-friendly navigation experience.

Key changes in this commit include:

1. The addition of the `registerCollapsibleNavHeader` method to the
ChromeService class.
2. Appropriate updates to tests and typings to support the newly
introduced functionality.

Signed-off-by: Yulong Ruan <[email protected]>
@ruanyl ruanyl force-pushed the feature/chrome-service-support-custom-menu-header branch from 42a07de to e973515 Compare December 13, 2023 03:27
@@ -941,4 +942,4 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)

### 🔩 Tests

- Update caniuse to fix failed integration tests ([#2322](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/2322))
- Update caniuse to fix failed integration tests ([#2322](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/2322))
Copy link
Member

Choose a reason for hiding this comment

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

Nit: unnecessary change on this line I suppose.

SuZhou-Joe
SuZhou-Joe previously approved these changes Dec 13, 2023
ashwin-pc
ashwin-pc previously approved these changes Dec 18, 2023
@manasvinibs manasvinibs merged commit 5695f9b into opensearch-project:main Dec 19, 2023
69 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-5244-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 5695f9b3407354333010c79330d6ddefa30dc65a
# Push it to GitHub
git push --set-upstream origin backport/backport-5244-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-5244-to-2.x.

ruanyl added a commit to ruanyl/OpenSearch-Dashboards that referenced this pull request Apr 16, 2024
…search-project#5244)

* feat: Introduce `registerCollapsibleNavHeader` to ChromeService

This commit introduces an enhancement to the ChromeService class
within our core application. It adds a new method named
`registerCollapsibleNavHeader`, allowing plugins to customize the
rendering of the collapsible navigation header in the global chrome UI.

With this new capability, plugins can now register their own
rendering logic for the collapsible navigation header. This feature
enhances the extensibility of our core system, empowering plugins to
provide a more tailored and user-friendly navigation experience.

Key changes in this commit include:

1. The addition of the `registerCollapsibleNavHeader` method to the
ChromeService class.
2. Appropriate updates to tests and typings to support the newly
introduced functionality.

Signed-off-by: Yulong Ruan <[email protected]>

* Add changelog for custom nav menu header register

+ tests

Signed-off-by: Yulong Ruan <[email protected]>

* Add a warning message to console when custom nav bar header been overridden

Signed-off-by: Yulong Ruan <[email protected]>

* Add tests for registerCollapsibleNavHeader warning console output

Signed-off-by: Yulong Ruan <[email protected]>

---------

Signed-off-by: Yulong Ruan <[email protected]>
Signed-off-by: Manasvini B Suryanarayana <[email protected]>
Co-authored-by: Manasvini B Suryanarayana <[email protected]>
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.

[Workspace] Provides fundamental features from OSD core to support workspace development
7 participants