-
Notifications
You must be signed in to change notification settings - Fork 40
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
[BUG] Refactor context menu #118
Comments
opensearch-project/OpenSearch-Dashboards#4453 Modified the breadcrumbs from the dashboard plugin, to be consistent with the actual name of the application. We should consider not hard checking the name of the breadcrumb but really hooking into the navigation plugin that provides the ability to do what this logic is doing without being hardcoded. opensearch-project/dashboards-reporting#118 Signed-off-by: Kawika Avilla <[email protected]>
opensearch-project/OpenSearch-Dashboards#4453 Modified the breadcrumbs from the dashboard plugin, to be consistent with the actual name of the application. We should consider not hard checking the name of the breadcrumb but really hooking into the navigation plugin that provides the ability to do what this logic is doing without being hardcoded. #118 Signed-off-by: Kawika Avilla <[email protected]>
opensearch-project/OpenSearch-Dashboards#4453 Modified the breadcrumbs from the dashboard plugin, to be consistent with the actual name of the application. We should consider not hard checking the name of the breadcrumb but really hooking into the navigation plugin that provides the ability to do what this logic is doing without being hardcoded. #118 Signed-off-by: Kawika Avilla <[email protected]> (cherry picked from commit 577871d)
opensearch-project/OpenSearch-Dashboards#4453 Modified the breadcrumbs from the dashboard plugin, to be consistent with the actual name of the application. We should consider not hard checking the name of the breadcrumb but really hooking into the navigation plugin that provides the ability to do what this logic is doing without being hardcoded. #118 Signed-off-by: Kawika Avilla <[email protected]> (cherry picked from commit 577871d)
opensearch-project/OpenSearch-Dashboards#4453 Modified the breadcrumbs from the dashboard plugin, to be consistent with the actual name of the application. We should consider not hard checking the name of the breadcrumb but really hooking into the navigation plugin that provides the ability to do what this logic is doing without being hardcoded. #118 Signed-off-by: Kawika Avilla <[email protected]> (cherry picked from commit 577871d) Co-authored-by: Kawika Avilla <[email protected]>
opensearch-project/OpenSearch-Dashboards#4453 Modified the breadcrumbs from the dashboard plugin, to be consistent with the actual name of the application. We should consider not hard checking the name of the breadcrumb but really hooking into the navigation plugin that provides the ability to do what this logic is doing without being hardcoded. #118 Signed-off-by: Kawika Avilla <[email protected]> (cherry picked from commit 577871d) Co-authored-by: Kawika Avilla <[email protected]>
See https://github.com/opensearch-project/OpenSearch-Dashboards/blob/main/test/plugin_functional/plugins/osd_top_nav/public/plugin.tsx for an example of how to correctly register menu items via |
Tagging this issue with 2.11 so it gets prioritized before next release. Temporary fix was already merged to 2.10 |
This was broken due to a CCI contribution in 2.10 that was not tested with the reporting plugin. The issues here is in the pipeline as an enhancement to the code. New feature should not break existing implementations. We will prioritize this as work in order. |
@anirudha this is more of a bug than an enhancement for sure and I wouldn't blame the CCI contribution. The way the context menu item is added is not using standard APIs and instead relies on css selectors that will definitely change in a minor or even patch release without notice. This has happened in 2.10 with the CCI improvement and again in the 2.11 release when we simply changed the top nav to remove the legacy discover toggle. It will likely again break in 2.12 and beyond unless fixed. Additionally we should not expect platform code to validate it's changes against plugins as that's not scalable. We have quite a lot of plugins and that is not to mention 3rd party plugins. OSD already respects semver for all the explicit and implicit APIs that plugins today use. We can't expect platforms to also validate every change against all the plugins too. |
Core services has There are two options for registering menu :
|
What is the bug?
The context menu component is very brittle and dependent on a number of OpenSearch Dashboards side effects that are not part of any public API. As OpenSearch Dashboards is refactored for OUI compliance, cohesion, and look and feel improvements, many of these hard-coded assumptions will break, leaving the current code non-functional.
How can one reproduce the bug?
A few, non-comprehensive examples:
dashboards-reporting/public/components/context_menu/context_menu.js
Lines 255 to 276 in c8be18e
navigation
plugin APIs. We will remove at least one of these class names soon (via [CCI] Remove unused tags in the navigation plugin OpenSearch-Dashboards#3964):dashboards-reporting/public/components/context_menu/context_menu.js
Lines 280 to 282 in c8be18e
dashboards-reporting/public/components/context_menu/context_menu_ui.js
Line 35 in c8be18e
What is the expected behavior?
Because the mentioned files don't use proper APIs, they may break at any time. They should be refactored to use interfaces that are subject to semver.
The text was updated successfully, but these errors were encountered: