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

Replace spy panels by Inspector #16387

Merged
merged 83 commits into from
Jun 20, 2018
Merged

Replace spy panels by Inspector #16387

merged 83 commits into from
Jun 20, 2018

Conversation

timroes
Copy link
Contributor

@timroes timroes commented Jan 30, 2018

close #16544, close #16417, close #15812, close #14642, close #13137, close #13038, close #12591, close #12552, close #5251, close #16185, close #15901, close #15897, close #13597, close #11470, close #8430, close #5116, close #13571, close #19658, close #19926

Replaces the previous spy-panels by the new Inspector (written in React). It's a contextual flyout (i.e. not limited to the visualization size anymore) to inspect data about specific elements (e.g. visualizations). The architecture of the Inspector was build flexible, so it's not limited to visualizations. Everything, that passes a list of "adapters" can open an inspector (e.g. we can make discover log it's requests in a request adapter and allow the user to view them in the inspector).

In contrast to the old spy panels the state of the inspector is not stored in the URL. It's a temporary view to inspect visualizations and not meant for sharing. If you actually used spy panels to e.g. show a table on a dashboard besides a visualization, we recommend just creating that table using the same aggregations as the visualization, and place it beside it on the dashboard.

We also move the trigger to open the Inspector out of the visualization.
You can currently open the inspector as follows:

  • On a Dashboard - Use the context menu of the panel and hit Inspector to open it.
  • In Visualization Editor - Use the "Inspector" button on top of the screen.

TODOs

  • Finalize design with @cchaos
    • Request selector design
  • Exchange dashboard panel action icon (new icon already in place, just need to wait for next EUI release)
  • Close inspectors when navigating away from Dashboards properly
  • Remove artificial delays/errors for async loading
  • Add more tests
  • Cross-read all texts
  • Fix sorting (blocked by Allow consumers to customize the sorting logic of the Table components eui#425)
  • Extract tabify code from courier request handler to inspector utils
  • Store table page size when reloading data

To test

To test this manually I would recommend the following setup:

  • Check if the inspector can be opened in visualization editor for a classical chart
  • Check if the inspector button is disabled in visualization editor mode for charts without inspector (e.g. TSVB, Vega)
  • Open the inspector on a dashboard panel (Icon should not exist for e.g. TSVB or Vega)
  • I would recommend creating the following charts to see all features:
    • A regular classical chart -> data and requests should be available
    • A chart containing a terms aggregation with "Other bucket Option" -> request panel will now show two requests

Screenshots

Inspector data panel

Inspector requests panel

Follow-ups (not fixed in this PR)

  • Properly show empty state in data table (issue with tabify) will be handled via tabify refactoring after merge
  • Don't use searchSource.history to get last request I will create a separate PR after this, since it will require some searchSource changes, which I would prefer to have in a separate PR

Dev-Docs

Inspector

The Inspector replaces spy panels, leading to a couple of changes:

  • There is no longer a showSpyPanel parameter to the visualize loader, because the vis internal button to open the spy panel no longer exists. To show an Inspector for an embedded visualization, use the openInspector() method on the visualize loader’s return value.
  • Spy modes and the “spyModes” app extension no longer exist. See the Inspector README for information on how to write custom Inspector views and adapters.

@timroes timroes added release_note:enhancement WIP Work in progress Feature:Visualizations Generic visualization features (in case no more specific feature label is available) labels Jan 30, 2018
@timroes
Copy link
Contributor Author

timroes commented Jan 30, 2018

As discusses in the meeting yesterday:

The inspector will be a flyout for now, though we want to look (separately from this feature) into a proper "data besides visualization" view for visualizations.

I would be interested in the following notes from @snide:

  • Could you check how the overall design of the inspector should be fixed.
  • I have currently two different mode selectors (see screenshots 3 and 4 above). Which one should we use, or should we implement that completely different?
  • Do you have any ideas on how to improve the request selection in the requests panel?

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@alexfrancoeur
Copy link

@timroes Just took this for a spin, noticed a few things that seemed off but I'm not sure if this PR is ready for early feedback. Is it worth getting feedback now or would you like me to hold off?

@timroes
Copy link
Contributor Author

timroes commented Apr 10, 2018

@alexfrancoeur I moved my TODO list to this PR above, so that you can get a feeling what's anyway still on my list. Please feel free to already provide early feedback, especially if you don't find it on the list above.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@alexfrancoeur
Copy link

@timroes b-e-a-utiful, will dig in further today 😄

@elasticmachine
Copy link
Contributor

💔 Build Failed

@alexfrancoeur
Copy link

@timroes some initial early, early feedback below. A few of these items are listed in your to do's already, but I figured I'd make note of them anyway. Hope this helps!

screen shot 2018-04-10 at 2 30 07 pm

I'm not sure if we've thought about the placement of the icon at all, but it's a bit too hidden. A number of users cannot find it. I think we should either move the magnifying glass to be near the context menu in the top right of the visualization on a dashboard or even adding it to the context menu directly. Thoughts? "View underlying data" as an option from this dropdown.

screen shot 2018-04-10 at 2 30 19 pm

The new visualization info button is a bit oddly places. It looks like I'm questioning the title of my viz. We could probably use design input here

screen shot 2018-04-10 at 2 30 33 pm

We discussed this briefly, I know we want to possibly remove "mode" here in favor of "view". Or we could simply have "Select" or no text at all. Some design input would be good here as well.

I know you mentioned this above, but we'll need to replace the requests icon.

screen shot 2018-04-10 at 2 30 59 pm

If there is only one request do we need the carrot? And if so, I believe this should be expanded rather than contracted.

screen shot 2018-04-10 at 2 31 17 pm

What indicates the color of the request? Are there static thresholds?

screen shot 2018-04-10 at 2 32 45 pm

The tooltip is a bit oddly place, may want to sync with design on the best approach here.

screen shot 2018-04-10 at 2 32 49 pm

Is there any way we can better identify the "data request"? Maybe with the index pattern name (logstash-* request)? Or visualization name + index pattern name (Unique Visitors: logstash-* request?

screen shot 2018-04-10 at 2 31 38 pm

Also mentioned in the to do's but we should probably exit if you were to leave the initial view that you were inspecting.

screen shot 2018-04-10 at 2 32 59 pm

The magnifying glass is very hidden everywhere. The sharing team is planning to introduce a context menu or "more" option in the dashboard navigation. If this is adopted globally, I wonder if it makes sense provide a menu item to "view underlying data" for single entities when they're in their own application. For example, saved search or current discover search could have this in the top nav so it's more discoverable. Visualizations the same.

screen shot 2018-04-10 at 2 33 36 pm

screen shot 2018-04-10 at 2 34 23 pm

We could even add an option in the dashboard nav to list all requests for the dashboard. THAT would be cool 😎

screen shot 2018-04-10 at 2 34 57 pm

I know this is still a WIP, but if for any reason there is only one view available, can we hide the carrot and make sure the element isn't clickable? Might not be worth the effort but thought I'd share.

screen shot 2018-04-10 at 2 35 34 pm

@timroes
Copy link
Contributor Author

timroes commented Apr 11, 2018

@alexfrancoeur thanks a lot for the early feedback. As you already recognized most of that was anyway on the TODO list, so let me focus on the new topics:

The new visualization info button is a bit oddly places. It looks like I'm questioning the title of my viz. We could probably use design input here

Yeah I totally agree with you. I just recently added the visualization title there. The help icon made more sense when it always said "Inspector" there. I earlier had the help button on the right side beside the close button, what we could try again. But that's one of the things I will sync with design on - added it to design TODOs.

If there is only one request do we need the carrot? And if so, I believe this should be expanded rather than contracted.

I know that was anyway on the TODO list, but I want to comment shortly on this, so we don't forget that in the design process. Apparently the usage of a caret to indicate the currently viewed request is not ideal. There can be multiple requests and it's not an collapse expand. I tried an accordion beforehand for the requests, but that had some technical issues. But since I anyway don't like the style of the request list, we might try an accordion again and make the needed changes to EUI for that (lazy loading of accordion content).

What indicates the color of the request? Are there static thresholds?

It's just whether the request succeeded or failed (same as the icon before the time, since we should never indicate information just by color).

Is there any way we can better identify the "data request"? Maybe with the index pattern name (logstash-* request)? Or visualization name + index pattern name (Unique Visitors: logstash-* request?

We could add the index pattern, but we should still keep a title for it, which doesn't need to be "data request". If you make a chart with an Other bucket, both requests will go to the same index, but one will be for the main data, and one for the "Other" values. Btw, just in case that wasn't clear from the TODO list: of course all requests will beside the title anyway still get a description (like the other request already has).

I know this is still a WIP, but if for any reason there is only one view available, can we hide the carrot and make sure the element isn't clickable? Might not be worth the effort but thought I'd share.

Not much of an effort and a good idea, so I added it to the TODO list. Just want to decide on the actual view chooser first, since I don't want to implement this now for all alternatives :-)

And last but not least... just because I think it wasn't clear from the above list:

The magnifying glass is very hidden everywhere. The sharing team is planning to introduce a context menu or "more" option in the dashboard navigation. If this is adopted globally, I wonder if it makes sense provide a menu item to "view underlying data" for single entities when they're in their own application. For example, saved search or current discover search could have this in the top nav so it's more discoverable. Visualizations the same.

Of course the magnifying glass is at all the planned solution. That's the very first large topic on the TODO list to implement the dashboard trigger, but that still requires another PR to be ready. And if you head over to Visualizations, you will already see the "Inspector" in the menu on the top - (maybe you've tested an older version?).

@elastic elastic deleted a comment from elasticmachine May 9, 2018
@elastic elastic deleted a comment from elasticmachine May 9, 2018
@elastic elastic deleted a comment from elasticmachine May 9, 2018
@elastic elastic deleted a comment from elasticmachine May 9, 2018
@elastic elastic deleted a comment from elasticmachine May 9, 2018
@elastic elastic deleted a comment from elasticmachine May 9, 2018
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@timroes
Copy link
Contributor Author

timroes commented Jun 19, 2018

Now is the time for final reviews! I am going to merge this tomorrow if noone vetoes against it and all tests pass.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@timroes
Copy link
Contributor Author

timroes commented Jun 19, 2018

Jenkins, test this - unrelated x-pack proc issue

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@timroes timroes removed the blocked label Jun 20, 2018
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@timroes timroes merged commit 2885e87 into elastic:master Jun 20, 2018
@timroes timroes deleted the inspector branch June 20, 2018 09:08
timroes added a commit to timroes/kibana that referenced this pull request Jun 20, 2018
* Add Inspector feature

* So long, and thanks for all the fish, spy panel

* Fix several functional tests

* Fix unit tests

* Fix spy panel button tests

* Replace old spy panel documentation

* Disable test temporarily until we have dashboard triggers

* Enter edit mode for dark theme test

* Fix some more functional tests

* Fix more functional tests

* More test fixing

* Fix more functional tests

* Allow opening the inspector via loader handler

* Refactor InspectorViewChooser, remove unused CSS

* Remove dead code

* Fix data download button style

* Remove redundant code

* Load inspectorViews for dashboard_viewer

* Extract inspector views to custom core_plugin

* Switch API to TypeScript 🎉

* Design changes

* Remove icons from views

* Design changes

* Improve typings of API

* Add typing to all adapters

* Show loading spinner in request selector

* Rewrite InspectorView to TypeScript

* Fix help text for data view

* Remove deprecated React lifecycle methods

* Embed inspector into dashboard panel actions

* Remove temporary inspector trigger

* Remove old CSS

* Fix dashboard trigger for new panel action

* Add tests for InspectorPanel and DataAdapter

* Produce a hierarchical table if the vis is hierarchical

* Remove allowJs option again

* Add missing Apache license headers

* Close inspector on dashboard when navigating away

* Use proper title for dashboard panels

* Fix functional tests

* Skip broken test for now

* Flush view chooser button

* Add request adapter tests

* Skip more tests, broken due to typescript

* Add Request Time description

* Add description for courier request

* Fix tests

* Replace icon by new (not yet released) icon

* Finalize design of inspector

* Remove discover test, that relied on spy panels

* Change API to be properly mockable in tests

* Add aria-live region for request status

* Replace old method in functional tests

* Replace abitrary magic number

* Use object destructuring in vis

* Fix issue with crashing requests view

* Add request time tooltip

* Get request body of correct search source

* Make filter buttons properly keyboard accessible

* Follow Dave's design suggestions

* Remove redundant request from name

* Remove unneeded comments

* WIP raw-formatted values

* Fix filtering issue

* Fix tests and more license headers

* Add data view tests

* Remove search from table

* Fix typos

* Implement review suggestion

* Remove artificial delays for testing

* Fix new panel action structure

* Minor design adjustments

* Fix failing functional test

* Update failing snapshot test

* Implement final wording

* Apply new EUI styling

* Fix closing inspector in tests

* Fix sorting of table

* Align punctuation between tooltips
timroes added a commit to timroes/kibana that referenced this pull request Jun 20, 2018
* Add Inspector feature

* So long, and thanks for all the fish, spy panel

* Fix several functional tests

* Fix unit tests

* Fix spy panel button tests

* Replace old spy panel documentation

* Disable test temporarily until we have dashboard triggers

* Enter edit mode for dark theme test

* Fix some more functional tests

* Fix more functional tests

* More test fixing

* Fix more functional tests

* Allow opening the inspector via loader handler

* Refactor InspectorViewChooser, remove unused CSS

* Remove dead code

* Fix data download button style

* Remove redundant code

* Load inspectorViews for dashboard_viewer

* Extract inspector views to custom core_plugin

* Switch API to TypeScript 🎉

* Design changes

* Remove icons from views

* Design changes

* Improve typings of API

* Add typing to all adapters

* Show loading spinner in request selector

* Rewrite InspectorView to TypeScript

* Fix help text for data view

* Remove deprecated React lifecycle methods

* Embed inspector into dashboard panel actions

* Remove temporary inspector trigger

* Remove old CSS

* Fix dashboard trigger for new panel action

* Add tests for InspectorPanel and DataAdapter

* Produce a hierarchical table if the vis is hierarchical

* Remove allowJs option again

* Add missing Apache license headers

* Close inspector on dashboard when navigating away

* Use proper title for dashboard panels

* Fix functional tests

* Skip broken test for now

* Flush view chooser button

* Add request adapter tests

* Skip more tests, broken due to typescript

* Add Request Time description

* Add description for courier request

* Fix tests

* Replace icon by new (not yet released) icon

* Finalize design of inspector

* Remove discover test, that relied on spy panels

* Change API to be properly mockable in tests

* Add aria-live region for request status

* Replace old method in functional tests

* Replace abitrary magic number

* Use object destructuring in vis

* Fix issue with crashing requests view

* Add request time tooltip

* Get request body of correct search source

* Make filter buttons properly keyboard accessible

* Follow Dave's design suggestions

* Remove redundant request from name

* Remove unneeded comments

* WIP raw-formatted values

* Fix filtering issue

* Fix tests and more license headers

* Add data view tests

* Remove search from table

* Fix typos

* Implement review suggestion

* Remove artificial delays for testing

* Fix new panel action structure

* Minor design adjustments

* Fix failing functional test

* Update failing snapshot test

* Implement final wording

* Apply new EUI styling

* Fix closing inspector in tests

* Fix sorting of table

* Align punctuation between tooltips
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

timroes added a commit to timroes/kibana that referenced this pull request Jun 20, 2018
* Add Inspector feature

* So long, and thanks for all the fish, spy panel

* Fix several functional tests

* Fix unit tests

* Fix spy panel button tests

* Replace old spy panel documentation

* Disable test temporarily until we have dashboard triggers

* Enter edit mode for dark theme test

* Fix some more functional tests

* Fix more functional tests

* More test fixing

* Fix more functional tests

* Allow opening the inspector via loader handler

* Refactor InspectorViewChooser, remove unused CSS

* Remove dead code

* Fix data download button style

* Remove redundant code

* Load inspectorViews for dashboard_viewer

* Extract inspector views to custom core_plugin

* Switch API to TypeScript 🎉

* Design changes

* Remove icons from views

* Design changes

* Improve typings of API

* Add typing to all adapters

* Show loading spinner in request selector

* Rewrite InspectorView to TypeScript

* Fix help text for data view

* Remove deprecated React lifecycle methods

* Embed inspector into dashboard panel actions

* Remove temporary inspector trigger

* Remove old CSS

* Fix dashboard trigger for new panel action

* Add tests for InspectorPanel and DataAdapter

* Produce a hierarchical table if the vis is hierarchical

* Remove allowJs option again

* Add missing Apache license headers

* Close inspector on dashboard when navigating away

* Use proper title for dashboard panels

* Fix functional tests

* Skip broken test for now

* Flush view chooser button

* Add request adapter tests

* Skip more tests, broken due to typescript

* Add Request Time description

* Add description for courier request

* Fix tests

* Replace icon by new (not yet released) icon

* Finalize design of inspector

* Remove discover test, that relied on spy panels

* Change API to be properly mockable in tests

* Add aria-live region for request status

* Replace old method in functional tests

* Replace abitrary magic number

* Use object destructuring in vis

* Fix issue with crashing requests view

* Add request time tooltip

* Get request body of correct search source

* Make filter buttons properly keyboard accessible

* Follow Dave's design suggestions

* Remove redundant request from name

* Remove unneeded comments

* WIP raw-formatted values

* Fix filtering issue

* Fix tests and more license headers

* Add data view tests

* Remove search from table

* Fix typos

* Implement review suggestion

* Remove artificial delays for testing

* Fix new panel action structure

* Minor design adjustments

* Fix failing functional test

* Update failing snapshot test

* Implement final wording

* Apply new EUI styling

* Fix closing inspector in tests

* Fix sorting of table

* Align punctuation between tooltips
timroes added a commit that referenced this pull request Jun 21, 2018
…st (#20072) (#20076)

* Replace spy panels by Inspector (#16387)

* Add Inspector feature

* So long, and thanks for all the fish, spy panel

* Fix several functional tests

* Fix unit tests

* Fix spy panel button tests

* Replace old spy panel documentation

* Disable test temporarily until we have dashboard triggers

* Enter edit mode for dark theme test

* Fix some more functional tests

* Fix more functional tests

* More test fixing

* Fix more functional tests

* Allow opening the inspector via loader handler

* Refactor InspectorViewChooser, remove unused CSS

* Remove dead code

* Fix data download button style

* Remove redundant code

* Load inspectorViews for dashboard_viewer

* Extract inspector views to custom core_plugin

* Switch API to TypeScript 🎉

* Design changes

* Remove icons from views

* Design changes

* Improve typings of API

* Add typing to all adapters

* Show loading spinner in request selector

* Rewrite InspectorView to TypeScript

* Fix help text for data view

* Remove deprecated React lifecycle methods

* Embed inspector into dashboard panel actions

* Remove temporary inspector trigger

* Remove old CSS

* Fix dashboard trigger for new panel action

* Add tests for InspectorPanel and DataAdapter

* Produce a hierarchical table if the vis is hierarchical

* Remove allowJs option again

* Add missing Apache license headers

* Close inspector on dashboard when navigating away

* Use proper title for dashboard panels

* Fix functional tests

* Skip broken test for now

* Flush view chooser button

* Add request adapter tests

* Skip more tests, broken due to typescript

* Add Request Time description

* Add description for courier request

* Fix tests

* Replace icon by new (not yet released) icon

* Finalize design of inspector

* Remove discover test, that relied on spy panels

* Change API to be properly mockable in tests

* Add aria-live region for request status

* Replace old method in functional tests

* Replace abitrary magic number

* Use object destructuring in vis

* Fix issue with crashing requests view

* Add request time tooltip

* Get request body of correct search source

* Make filter buttons properly keyboard accessible

* Follow Dave's design suggestions

* Remove redundant request from name

* Remove unneeded comments

* WIP raw-formatted values

* Fix filtering issue

* Fix tests and more license headers

* Add data view tests

* Remove search from table

* Fix typos

* Implement review suggestion

* Remove artificial delays for testing

* Fix new panel action structure

* Minor design adjustments

* Fix failing functional test

* Update failing snapshot test

* Implement final wording

* Apply new EUI styling

* Fix closing inspector in tests

* Fix sorting of table

* Align punctuation between tooltips

* Fix test that doesn't exist on master

* Fix one inspector test (#20072)

* Remove file that came accidentally back due to merging

* Fix accidental merge paste

* Fix 6.4 test failures
maryia-lapata pushed a commit to maryia-lapata/kibana that referenced this pull request Jun 25, 2018
* Add Inspector feature

* So long, and thanks for all the fish, spy panel

* Fix several functional tests

* Fix unit tests

* Fix spy panel button tests

* Replace old spy panel documentation

* Disable test temporarily until we have dashboard triggers

* Enter edit mode for dark theme test

* Fix some more functional tests

* Fix more functional tests

* More test fixing

* Fix more functional tests

* Allow opening the inspector via loader handler

* Refactor InspectorViewChooser, remove unused CSS

* Remove dead code

* Fix data download button style

* Remove redundant code

* Load inspectorViews for dashboard_viewer

* Extract inspector views to custom core_plugin

* Switch API to TypeScript 🎉

* Design changes

* Remove icons from views

* Design changes

* Improve typings of API

* Add typing to all adapters

* Show loading spinner in request selector

* Rewrite InspectorView to TypeScript

* Fix help text for data view

* Remove deprecated React lifecycle methods

* Embed inspector into dashboard panel actions

* Remove temporary inspector trigger

* Remove old CSS

* Fix dashboard trigger for new panel action

* Add tests for InspectorPanel and DataAdapter

* Produce a hierarchical table if the vis is hierarchical

* Remove allowJs option again

* Add missing Apache license headers

* Close inspector on dashboard when navigating away

* Use proper title for dashboard panels

* Fix functional tests

* Skip broken test for now

* Flush view chooser button

* Add request adapter tests

* Skip more tests, broken due to typescript

* Add Request Time description

* Add description for courier request

* Fix tests

* Replace icon by new (not yet released) icon

* Finalize design of inspector

* Remove discover test, that relied on spy panels

* Change API to be properly mockable in tests

* Add aria-live region for request status

* Replace old method in functional tests

* Replace abitrary magic number

* Use object destructuring in vis

* Fix issue with crashing requests view

* Add request time tooltip

* Get request body of correct search source

* Make filter buttons properly keyboard accessible

* Follow Dave's design suggestions

* Remove redundant request from name

* Remove unneeded comments

* WIP raw-formatted values

* Fix filtering issue

* Fix tests and more license headers

* Add data view tests

* Remove search from table

* Fix typos

* Implement review suggestion

* Remove artificial delays for testing

* Fix new panel action structure

* Minor design adjustments

* Fix failing functional test

* Update failing snapshot test

* Implement final wording

* Apply new EUI styling

* Fix closing inspector in tests

* Fix sorting of table

* Align punctuation between tooltips
@timroes timroes added the release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. label Aug 7, 2018
@bhavyarm bhavyarm mentioned this pull request Aug 22, 2018
16 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Inspector Inspector infrastructure and implementations Feature:Visualizations Generic visualization features (in case no more specific feature label is available) release_note:enhancement release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. review v6.4.0 v7.0.0
Projects
None yet
9 participants