-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
Redesign the "Add Panel" Experience #183764
Conversation
bd2aeab
to
c564ea6
Compare
} | ||
|
||
public async isCompatible() { | ||
return true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need further clarification about this, I'm assuming it would be compatible since this action only calls a function to display the selection modal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would check that at least apiCanAddNewPanel(embeddable)
is true to make sure the embeddable is capable of adding new panels.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI you would be checking to ensure the parent API is compatible - so something like apiHasParentApi(embeddable) && apiCanAddNewPanel(embeddable.parentApi)
:)
e3a5e7c
to
b5180a7
Compare
/ci |
6d80484
to
50422f5
Compare
/ci |
1 similar comment
/ci |
0727cd2
to
a4fd21c
Compare
/ci |
a4fd21c
to
dfe8c0d
Compare
/ci |
dfe8c0d
to
8a41342
Compare
src/plugins/ui_actions/public/context_menu/build_eui_context_menu_panels.tsx
Outdated
Show resolved
Hide resolved
|
||
if (!isServerless) { | ||
// only register these actions in stateful kibana | ||
uiActions.addTriggerAction(ADD_PANEL_TRIGGER, addOverviewPanelAction); | ||
uiActions.addTriggerAction(ADD_PANEL_TRIGGER, addErrorBudgetPanelAction); | ||
uiActions.addTriggerAction(ADD_PANEL_TRIGGER, addAlertsPanelAction); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
exclude the registering of observability SLO actions in serverless.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eokoneyo Can you elaborate more why you had to exclude these actions on serverless?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eokoneyo I just noticed that this file is outdated and you indeed register these actions on stateful and observability serverless. In the beginning I thought you were disabling on serverless just the SLO edit
actions, but that was my misunderstanding.
8d75eed
to
2c75629
Compare
/ci |
2c75629
to
7bf242c
Compare
/ci |
7bf242c
to
5906d1d
Compare
/ci |
UX change: I changed the add panel flyout search behavior to hiding the sections that don't match the search string. This resolves an issue with a failing maps test where the flyout was scrolling to the bottom of the flyout which prevented the maps panel option from being clicked. Tiny UI change: I added an empty state for when no panel types match your search string. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A lot of this is looking good! Tested it locally and it also works great - especially the new searching.
I thought that we had removed all of the placementPriority
code, but I still see a lot of references to it. I'd really like to have this whole system removed, as the actions already contain an order
param.
Is this possible?
return isApiCompatible(embeddable); | ||
} | ||
|
||
public execute({ embeddable }: EmbeddableApiContext): Promise<void> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: why not use an async function here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in 48bef80
return apiHasType(api) && api.type === 'dashboard'; | ||
}; | ||
|
||
export class AddAggVisualizationPanelAction implements Action<EmbeddableApiContext> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason this action is in the Dashboard plugin and not in the Visualize plugin?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved this to the visualizations plugin in 48bef80
items: PanelSelectionMenuItem[]; | ||
}; | ||
|
||
export interface PlacementPriority { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought we removed this placementPriority
concept in favour of the order
key? If so, can we clean these up? If they are still used, I'd like to move away from them because this info should be captured in the normal action types.
edit it looks like these are still used in quite a few places. See top level review comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed all references to placementPriority
in 910065f.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks much better! I tested locally again, and found a very small regression - if you open an agg based editor now and hit cancel
you'll end up at /app/dashboard
which is a 404 page. I wasn't able to reproduce this in main.
Another small detail we could consider - perhaps in a followup, is that oftentimes the editors need async-loaded code, or take some time to open. This means that the add panel flyout closes immediately, and then the editor for the chart type you selected takes a moment to appear. We could handle this load state more gracefully - this is especially noticeable with the ES|QL editor.
|
||
import { i18n } from '@kbn/i18n'; | ||
|
||
export const COMMON_EMBEDDABLE_GROUPING = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: can we type these? Maybe UiActionsPresentableGrouping<unknown>
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the type in 5337820
I was passing the wrong |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes LGTM!
Ran it locally on chrome, ensuring that all add panel actions worked as expected. I think it might be worth creating an issue for the ability to wait for an editor to be ready before closing the add panel menu - because it was jarring a few times when the flyout closed and there was a visible delay in opening the next step.
I also looked through the code, and it all seems functional. IMO, there are a lot of ways it could be made more concise or organized, but likely most of this cleanup will happen after the embeddable refactor, once we've removed the legacy factories.
x-pack/plugins/aiops/public/ui_actions/create_change_point_chart.tsx
Outdated
Show resolved
Hide resolved
Noted when testing this for ML that the ML panel types are still shown even when the ML feature is |
💛 Build succeeded, but was flaky
Failed CI StepsMetrics [docs]Module Count
Public APIs missing comments
Async chunks
Page load bundle
Unknown metric groupsAPI count
ESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: cc @eokoneyo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change for ML LGTM. Thanks for switching the order of the items @cqliu1
## Summary Follow up to #183764. The labels for the Visualizations and Observability groupings shown in the Add Panel flyout in Dashboard were not being translated. ### Checklist Delete any items that are not applicable to this PR. - [ ] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md) - [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [ ] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [ ] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed - [ ] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/)) - [ ] Any UI touched in this PR does not create any new axe failures (run axe in browser: [FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/), [Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US)) - [ ] If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the [docker list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker) - [ ] This renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server)) - [ ] This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers) ### Risk Matrix Delete this section if it is not applicable to this PR. Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release. When forming the risk matrix, consider some of the following examples and how they may potentially impact the change: | Risk | Probability | Severity | Mitigation/Notes | |---------------------------|-------------|----------|-------------------------| | Multiple Spaces—unexpected behavior in non-default Kibana Space. | Low | High | Integration tests will verify that all features are still supported in non-default Kibana Space and when user switches between spaces. | | Multiple nodes—Elasticsearch polling might have race conditions when multiple Kibana nodes are polling for the same tasks. | High | Low | Tasks are idempotent, so executing them multiple times will not result in logical error, but will degrade performance. To test for this case we add plenty of unit tests around this logic and document manual testing procedure. | | Code should gracefully handle cases when feature X or plugin Y are disabled. | Medium | High | Unit tests will verify that any feature flag or plugin combination still results in our service operational. | | [See more potential risk examples](https://github.com/elastic/kibana/blob/main/RISK_MATRIX.mdx) | ### For maintainers - [ ] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)
Summary
Closes #144418
This PR introduces changes to the dashboard add panel selection functionality, so that panel selection would now happen from within a flyout, and as such panels are now grouped together logically.
With this implementation any panel that is intended to show up within this new flyout is required to have either been registered leveraging the ui action trigger
ADD_PANEL_TRIGGER
and have it'sgrouping
value defined or belong to a subset of visualization types (PROMOTED
,TOOLS
, andLEGACY
) that would automatically get grouped.It's worth pointing out that because we can't control the order at which UI actions gets registered, we won't always get the the panel groups in the same order, for this specific reason
a new optional property (the propertyplacementPriority
) has been added inorder
is now leveraged such that it allows a user registering a UI action define a relative weight for where they'd like their group to show up. All registered actions would be rendered in descending order considering allorder
defined, in the case where no order is defined0
is assumed for the group. In addition an action which is registered without a group, would automatically get assigned into a default group titled "Other".The search implemented within the add panel is rudimentary, checking if the group titles and group item titles contain the input character; when a group title is matched the entire group is remains highlighted, in the case that the group isn't matched and it's just the group item, only said item is highlighted within it's group.
Visuals
Default view
Search match view
P.S.
This changes also includes changes to the display of certain panels;
Checklist
Delete any items that are not applicable to this PR.