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 registrable DiscoverActions to discover page #7793

Closed

Conversation

gaobinlong
Copy link
Contributor

@gaobinlong gaobinlong commented Aug 22, 2024

Description

This PR adds a registrable DiscoverActions component to DiscoverCanvas in the discover page, so that other plugins can register their own actions, and then display the actions in the Discover page.

Issues Resolved

#7786

Screenshot

image

Testing the changes

If no actions are registered, then no impact on the discover page.
All of the registered actions will be associated with their own feature flags, if users don't enable the feature flags, then no actions will be shown in the discover page.

Changelog

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

Copy link
Contributor

ℹ️ Manual Changeset Creation Reminder

Please ensure manual commit for changeset file 7793.yml under folder changelogs/fragments to complete this PR.

If you want to use the available OpenSearch Changeset Bot App to avoid manual creation of changeset file you can install it in your forked repository following this link.

For more information about formatting of changeset files, please visit OpenSearch Auto Changeset and Release Notes Tool.

@SuZhou-Joe
Copy link
Member

SuZhou-Joe commented Aug 22, 2024

Do not know if we have taken a look on the existing features of ui_actions_service.

https://github.com/opensearch-project/OpenSearch-Dashboards/blob/cb273364086057b542a55d2bbe28631313a22df9/src/plugins/ui_actions/README.md

Copy link

codecov bot commented Aug 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 63.82%. Comparing base (a5882ee) to head (a70aace).
Report is 101 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #7793   +/-   ##
=======================================
  Coverage   63.82%   63.82%           
=======================================
  Files        3658     3659    +1     
  Lines       81268    81274    +6     
  Branches    12964    12964           
=======================================
+ Hits        51867    51873    +6     
  Misses      26219    26219           
  Partials     3182     3182           
Flag Coverage Δ
Linux_1 30.14% <ø> (ø)
Linux_2 55.87% <ø> (ø)
Linux_3 40.42% <100.00%> (?)
Linux_4 31.28% <ø> (ø)
Windows_1 30.16% <ø> (ø)
Windows_2 55.82% <ø> (ø)
Windows_3 40.42% <100.00%> (+<0.01%) ⬆️
Windows_4 31.28% <ø> (ø)

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.

Signed-off-by: gaobinlong <[email protected]>
@gaobinlong
Copy link
Contributor Author

Do not know if we have taken a look on the existing features of ui_actions_service.

https://github.com/opensearch-project/OpenSearch-> Dashboards/blob/cb273364086057b542a55d2bbe28631313a22df9/src/plugins/ui_actions/README.m

Yeah, we have considered that solution but it doesn't meet our requirement, ui actions only supports a menu button with multiple actions, the style is different from the UI mockup for generate anomaly detector and text to visualization which flatten all of the actions in the page, so we cannot use ui actions directly.
image

@AMoo-Miki
Copy link
Collaborator

Yeah, we have considered that solution but it doesn't meet our requirement, ui actions only supports a menu button with multiple actions, the style is different from the UI mockup for generate anomaly detector and text to visualization which flatten all of the actions in the page, so we cannot use ui actions directly.

If i understand correctly, all you need is a different appearance. What are your requirements specifically that are not supported by the existing system?

@gaobinlong
Copy link
Contributor Author

Yeah, we have considered that solution but it doesn't meet our requirement, ui actions only supports a menu button with multiple actions, the style is different from the UI mockup for generate anomaly detector and text to visualization which flatten all of the actions in the page, so we cannot use ui actions directly.

If i understand correctly, all you need is a different appearance. What are your requirements specifically that are not supported by the existing system?

I think that's because UI actions needs all registered actions behind a clickable element, when clicking that element, all actions are shown, something like a popover, this is different from our requirement that no clickable entry point exists, but show all the registered actions in the page directly. Maybe we can change the existing UI actions to make it support our case, but the effort is much more than the implementation in this PR.

@kavilla
Copy link
Member

kavilla commented Aug 22, 2024

this seems a little bit of an anti pattern.

Discover has a set of registered actions that it can do into the top nav bar
Screenshot 2024-05-22 at 2 32 16 PM

the other thing is that the histogram is apart of the discover plugin if and only if there is date histogram aggregation which only occurs if timestamps exist. Logically it makes sense for AD that if no timestamp then the action doesn't really matter since I doubt it is possible to do AD without timestamps but for any other type of action this will will not be preset if no timestamp field present.

i think if we really wanted something we could thing bigger and apply it with the query editor component. if you run yarn start:enhancements you will get the future iteration of the component.

@joshuali925 has made it really easy to portal in a component from the AD dashboards plugin to the query editor header bar:

#7034

@joshuali925
Copy link
Member

yeah I'm not sure if anomaly detector button should go into the query editor extensions UI, but from user perspective i feel it's a bit weird to have AD next to histogram, since actions are generally on top. It might break the experience if some plugins put actions on top and some put them below

@dagneyb
Copy link

dagneyb commented Aug 23, 2024

My two cents: I dont think this action belongs with the top nav "high-level" actions. The call to actions we are putting in line are truly intended to be in in-line with the associated data. We shouldn't let our current architecture drive our design.

I'm not opposed to putting these in the query editor component as @kavilla called out, but I think we may be up against the deadline to make that change for 2.17.

@ruanyl
Copy link
Member

ruanyl commented Sep 10, 2024

Closing as this is not needed

@ruanyl ruanyl closed this Sep 10, 2024
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