-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Controls] Dashboard Integration #115991
[Controls] Dashboard Integration #115991
Conversation
… creating new Control
5ba1815
to
3682356
Compare
Pinging @elastic/kibana-presentation (Team:Presentation) |
…dle size below original value
…same pattern in control group and panel
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.
Here's a recording where I get a single control into a weird state. Brand new dashboard. Add Control. Sampledata ecommerce/customer_gender Select Male Save Male is double selected. Switch to View Mode prompts about unsaved changes. Screen.Recording.2021-10-25.at.7.38.06.PM.mov |
@ThomThomson A couple more things I've found, one design related and the other one isn't.
|
…le is destroyed. Do not combine reducers when the reducer already exists. Destroy and unmount control group
@crob611, I was able to fix the issue that you noticed. It was caused by @andreadelrio, since searching through fields is done clientside, making the field search case insensitive should be pretty straightforward! Also, we definitely should make the data views picker wider, these are perfect candidates for the upcoming design review PR! I can start on some of these once this is merged. |
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.
Have looked over the code several times, and it looks good (just a couple of nits/comments below)
Tested functionality and it works great.
@@ -7,7 +7,7 @@ | |||
*/ | |||
|
|||
import { AddToLibraryAction } from '.'; | |||
import { DashboardContainer } from '../embeddable'; | |||
import { DashboardContainer } from '../embeddable/dashboard_container'; |
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.
What's the reason for only exporting the DashboardContainer type and not the entire class, thus having to change all of these imports to a deeper path?
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.
This is the secret to how I made the initial dashboard plugin bundle less than 100kb. The dashboard embeddable contains a whole bunch of code and resources, so I imported it async in the factory.
That alone cut the bundle size from 230kb to 80kb.
The only places that used the dashboard container class itself without going through the factory were the actions/tests so I just updated them all to import from elsewhere.
@@ -77,6 +81,14 @@ export const syncDashboardIndexPatterns = ({ | |||
}) | |||
); | |||
|
|||
if (dashboardContainer.controlGroup) { |
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: Could this be cleaned up a bit so you're not duping the .pipe.subscribe
?
Maybe always make an array of container output and add the control group if it's there and always do combine?
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.
Ah good call! I've cleaned it up using your suggestion
className="presFieldPicker__fieldButton" | ||
onClick={() => setSelectedField(f)} | ||
isActive={f.name === selectedField?.name} | ||
className={classNames('presFieldPicker__fieldButton', { |
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.
Can we use emotion for classNames instead of classnames?
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 can switch this out throughout the controls section in a follow-up - just gotta look into how it works, as it seems not to be 1 to 1.
💛 Build succeeded, but was flaky
Test FailuresMetrics [docs]Module Count
Public APIs missing comments
Any counts in public APIs
Async chunks
Public APIs missing exports
Page load bundle
Saved Objects .kibana field count
Unknown metric groupsAPI count
async chunk count
References to deprecated APIs
History
To update your PR or re-run it, just comment with: |
…-migrate-away-from-injected-css-js * 'master' of github.com:elastic/kibana: (61 commits) [ML] Nodes overview for the Model Management page (elastic#116361) [Uptime] Uptime index config using kibana.yml (elastic#115775) [Controls] Dashboard Integration (elastic#115991) skip flaky suite (elastic#104260) Include Files in GitHub UI (elastic#115956) skip flaky suite (elastic#116060) [Canvas] By-Value Embeddables (elastic#113827) Skip failing test (elastic#115366) [Osquery] Fix live query search doesn't return relevant results for agents (elastic#116332) [Integrations] Added link in old Add Data description and fixed alignment in cards (elastic#116213) [Actions] Extended ActionTypeRegistry with connector validation to validate config with secrets (elastic#116079) skip flaky suite (elastic#109329) Grant access to machine learning features when base privileges are used (elastic#115444) Skipping failing test (elastic#84957) [RAC][Security Solution] Adds migration to new SecuritySolution rule types (elastic#112113) skip flaky suite (elastic#115366) [Fleet] Marking API spec as experimental (elastic#116331) [Docs] Cleaning up the versions in the upgrade paths. Closes elastic#116223 (elastic#116228) [Reporting] Suppress debug logs in the mock logger (elastic#116012) [Metrics UI] Clear threshold alert groups state when filterQuery changes (elastic#116205) ... # Conflicts: # src/plugins/dashboard/public/application/embeddable/dashboard_container.tsx # src/plugins/dashboard/public/types.ts
This reverts commit 0b2dcdf.
Part of #99993
Summary
Integrates Phase 1 of the Controls project with dashboard. This includes the following broad tasks:
filters
,query
,timeRange
,lastReloadRequestTime
andviewMode
are passed from the dashboard to Control GroupHow to enable
You can find the dashboard controls advanced setting under the Presentation Labs header in Stack Management -> Advanced Settings.
Feature GIFs
Control selections apply to charts
Filters Query & Timerange apply to Controls
Edit and Save
Missing pieces for release
There are still a few missing pieces before this project is release-ready:
Functional testing
functional tests will be added soon, the tests which are needed to cover this project are tracked in #115816
Final design review
Using this PR as a basis for the look and feel of the Controls project integrated on dashboard, the design team will do a final once over to finalize the design.
Some broad upcoming design ideas include:
Important Phase 2 features
There are some missing features in this version of the Controls project that are not technically necessary for the first release, but can cause some UX problems and confusion.
Ability to ignore parent filtering
By default, the time range, filters, and query settings from the dashboard are passed down to the control group, and the individual controls. Making changes to the filters of the dashboard will force a refresh of the controls. This setting should be made configurable for the dashboard author, in case they want to allow their users to use both types of control. The experimental Input Controls have this feature, but only for the time range.
Automatic Validation
When a user has a selection in their control, and the context of the dashboard changes from for instance a change in the filters, query, time range, or refresh, the control will re-query, but it will not validate the user's selections. This can result in inconsistencies, as the current selection will not be listed in the available options.
Additionally, when filtering by the same field in the dashboard filters, and in a control, the control selections can sometimes override the filter selections. This is especially apparent when the user has selected two options in the control, then adds a filter to the dashboard narrowing the selections down to one. The control selections will override the dashboard selections and the dashboard will show results for both.
Automatic validation will take care of both of these issues. Whenever the control fetches its available options, it will also send a cardinality query for each of its selections. If the cardinality query returns 0 for any term, that term will be considered
errorSelected
instead ofselected
, and will be retained in the control, but will not be applied. This feature is necessary for hierarchical chaining.Missing Range Slider
The experimental Input Controls have a range slider control option which is completely missing in this iteration of the Controls project. Users may expect to be able to add a range slider control.
Missing non-search option for Options List
In the experimental Input Controls, users can add an options list which does not use typeahead functionality. Instead, the author defines a max number of terms to fetch. This option is completely missing from this iteration of the Controls project. Users seem to use this option often, and may be surprised that it is not yet included.
Options List Fields
due to our usage of the new autocomplete API, certain field types are not supported by the Options List Control. Currently the options list control only supports String and Boolean field types. The experimental Input Controls support selection of Date and Number fields as well.