opensearch-project / dashboards-reporting

Apache License 2.0
9 stars 26 forks source link

[BUG] Refactor context menu #118

Open joshuarrrr opened 1 year ago

joshuarrrr commented 1 year ago

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:

  1. These methods for determining the current app are all incorrect and brittle: https://github.com/opensearch-project/dashboards-reporting/blob/c8be18e1946a5b07451b7f0bcbb7444bdb8cf664/public/components/context_menu/context_menu.js#L255-L276
  2. The nav menu integration tries to use class names instead of navigation plugin APIs. We will remove at least one of these class names soon (via https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3964): https://github.com/opensearch-project/dashboards-reporting/blob/c8be18e1946a5b07451b7f0bcbb7444bdb8cf664/public/components/context_menu/context_menu.js#L280-L282
  3. HTML templates don't correctly position UI elements; UI controls must be rendered using actual OUI components: https://github.com/opensearch-project/dashboards-reporting/blob/c8be18e1946a5b07451b7f0bcbb7444bdb8cf664/public/components/context_menu/context_menu_ui.js#L35

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.

joshuarrrr commented 10 months ago

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 navigation.registerMenuItem().

derek-ho commented 10 months ago

Tagging this issue with 2.11 so it gets prioritized before next release. Temporary fix was already merged to 2.10

anirudha commented 9 months ago

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.

ashwin-pc commented 9 months ago

@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.

pjfitzgibbons commented 9 months ago

Core services has core.application.currentAppId$. This can be used for conditional on which "app" is currently rendering.

There are two options for registering menu :