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

[Logs Explorer] Hide hard coded flyout actions using customization extension point #166638

Merged

Conversation

tonyghiani
Copy link
Contributor

📓 Summary

Closes #165217

This work implements a new extension point in the Discover customization framework to allow customizing the flyout content. Although it enables customizing only the actions displayed on top, it could be enhanced for additional flyout customizations in the future.

To keep it simple and familiar to other customizations, it relies on a similar API as the top_nav customization, allowing one to disable default actions and insert additional ones.

/**
 * Hide flyout actions to prevent rendering hard-coded actions.
 */
customizations.set({
  id: 'flyout',
  actions: {
    defaultActions: {
      viewSingleDocument: { disabled: true },
      viewSurroundingDocument: { disabled: true },
    },
  },
});

🧪 Testing

  • Navigate to /app/observability-log-explorer and expand the flyout for any grid entry.
    The actions on top of the flyout should not be displayed, only the title and the pagination control should appear.
  • Navigate to /app/discover and expand the flyout for any grid entry.
    The actions for viewing a single documents or surrounding documents should be displayed.

@tonyghiani tonyghiani requested review from a team as code owners September 18, 2023 15:16
@tonyghiani tonyghiani added Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services release_note:skip Skip the PR/issue when compiling release notes labels Sep 18, 2023
@apmmachine
Copy link
Contributor

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

  • /oblt-deploy : Deploy a Kibana instance using the Observability test environments.
  • /oblt-deploy-serverless : Deploy a serverless Kibana instance using the Observability test environments.
  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@elasticmachine
Copy link
Contributor

Pinging @elastic/infra-monitoring-ui (Team:Infra Monitoring UI)

const { singleDocHref, contextViewHref, onOpenSingleDoc, onOpenContextView } = useNavigationProps(
{ dataView, rowIndex: hit.raw._index, rowId: hit.raw._id, columns, filters, savedSearchId }
);
const { flyoutActions } = useFlyoutActions({
Copy link
Contributor Author

Choose a reason for hiding this comment

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

note: this hook is purely helpful to split the computation of the actions list on top of the customization framework and the navigation helpers to keep cleaner the whole flyout rendering content.

href: string;
}

const staticViewDocumentItem = {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

note: this entry represents the static "View" label at the beginning of the list, which will be prepended only in the presence of some enabled actions.

Copy link
Contributor

@davismcphee davismcphee left a comment

Choose a reason for hiding this comment

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

This looks good to me and worked well in testing, but it looks like Security simultaneously added the same customization point in #166780. I think we should all align on one implementation before merging either.

One thing I like about this implementation is that it allows for selectively disabling the links if needed, and it provides a good base for future enhancements like a getActionItems() option.

@kqualters-elastic What do you think about merging this PR first and adapting your PR to this implementation of the flyout customization point?

id: 'flyout';
actions: {
defaultActions?: FlyoutDefaultActions;
getActionItems?: () => FlyoutActionItem[];
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
getActionItems?: () => FlyoutActionItem[];

This would make sense, but it doesn't look like it's implemented or used currently, so I think we should drop this for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, I totally forgot to apply the custom action items into the rendered list 🙈
Thanks for the heads up, instead of removing it I used it when computing the actions list, as we'll probably need to use it soon for our customizations and to keep it aligned with the options available for the top_nav customization (disabling default entries, add custom entries)

Copy link
Contributor

@mohamedhamed-ahmed mohamedhamed-ahmed left a comment

Choose a reason for hiding this comment

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

Infra changes LGTM, nice work!

Copy link
Contributor

@kqualters-elastic kqualters-elastic left a comment

Choose a reason for hiding this comment

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

independently did basically the same thing in #166780 so lgtm 😂 commenting so i get a notification when it's merged ha

@kqualters-elastic
Copy link
Contributor

@davismcphee ya that sounds good to me, will update once this is merged

Copy link
Contributor

@davismcphee davismcphee left a comment

Choose a reason for hiding this comment

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

The latest changes look good to me, and it seems like we're all in agreement on merging this implementation. Would be great to have a unit test for getActionItems too, but approving now since otherwise it all LGTM to me.

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
discover 651 653 +2

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
discover 72 75 +3

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
discover 555.5KB 557.8KB +2.3KB

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id before after diff
discover 15 17 +2

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
logExplorer 34.9KB 35.0KB +119.0B
Unknown metric groups

API count

id before after diff
discover 113 116 +3

ESLint disabled line counts

id before after diff
discover 24 22 -2

Total ESLint disabled count

id before after diff
discover 24 22 -2

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@tonyghiani tonyghiani merged commit 0743a11 into elastic:main Sep 21, 2023
@tonyghiani tonyghiani deleted the 165217-log-explorer-hardcoded-links branch September 21, 2023 11:50
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Sep 21, 2023
gergoabraham pushed a commit to gergoabraham/kibana that referenced this pull request Sep 21, 2023
…ension point (elastic#166638)

## 📓 Summary

Closes elastic#165217 

This work implements a new extension point in the Discover customization
framework to allow customizing the flyout content. Although it enables
customizing only the actions displayed on top, it could be enhanced for
additional flyout customizations in the future.

To keep it simple and familiar to other customizations, it relies on a
similar API as the `top_nav` customization, allowing one to disable
default actions and insert additional ones.

```ts
/**
 * Hide flyout actions to prevent rendering hard-coded actions.
 */
customizations.set({
  id: 'flyout',
  actions: {
    defaultActions: {
      viewSingleDocument: { disabled: true },
      viewSurroundingDocument: { disabled: true },
    },
  },
});
```

## 🧪 Testing 
- Navigate to `/app/observability-log-explorer` and expand the flyout
for any grid entry.
The actions on top of the flyout should not be displayed, only the title
and the pagination control should appear.
- Navigate to `/app/discover` and expand the flyout for any grid entry. 
The actions for viewing a single documents or surrounding documents
should be displayed.

---------

Co-authored-by: Marco Antonio Ghiani <[email protected]>
@achyutjhunjhunwala achyutjhunjhunwala changed the title [Log Explorer] Hide hard coded flyout actions using customization extension point [Logs Explorer] Hide hard coded flyout actions using customization extension point Jan 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services v8.11.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Explorer] Remove Discover links from log explorer document fly-out
8 participants