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.

Sign up for GitHub

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

Jump to bottom

Remove navigation from Discover to Visualize #89132

Merged
merged 14 commits into from
Feb 3, 2021
Merged

Remove navigation from Discover to Visualize #89132

merged 14 commits into from
Feb 3, 2021

Conversation

stratoula
Copy link
Contributor

@stratoula stratoula commented Jan 25, 2021 •
edited
Loading

Summary

This PR removes the Discover's ability to navigate to Visualize. This functionality was enabled only for the OSS distributable.

Specifically:

  • Removes the UI action from visualize plugin (and all the unused setters and getters). It was introduced here.
  • Removes the URL generator
  • Removes/skips the functional tests that runs for OSS

@stratoula
Copy link
Contributor Author

@elasticmachine merge upstream

@stratoula stratoula added v7.12.0 v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Feature:Discover Discover Application Feature:Vis Editor Visualization editor issues Team:Visualizations Visualization editors, elastic-charts and infrastructure labels Jan 26, 2021
@stratoula stratoula marked this pull request as ready for review January 26, 2021 12:20
@stratoula stratoula requested a review from a team January 26, 2021 12:20
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app (Team:KibanaApp)

@stratoula
Copy link
Contributor Author

@elasticmachine merge upstream

@stratoula
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

merge conflict between base and head

@stratoula
Copy link
Contributor Author

@elasticmachine merge upstream

sulemanof
sulemanof reviewed Feb 2, 2021
View reviewed changes
Copy link
Contributor

@sulemanof sulemanof left a comment

Choose a reason for hiding this comment

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

Code removing LGTM overall, a bit sad to see the Visualize sunset 😢
Left couple of questions to discuss.

The last question: do we need to change wording in
src/plugins/discover/public/application/components/sidebar/discover_field_details.tsx for Visualize button since it has no more connections to visualize app?
the same question for tracking clicks naming:
image

I don't have a good idea how to rename it, so maybe it should stay as it is ☺️

test/functional/apps/discover/_field_visualize.ts Show resolved Hide resolved
test/functional/apps/management/index.ts Outdated Show resolved Hide resolved
@kertal
Copy link
Member

kertal commented Feb 2, 2021 •
edited
Loading

Adding a few cents from Discover side

Code removing LGTM overall, a bit sad to see the Visualize sunset 😢

@sulemanof sharing your pain from Discover side, it was fun all these years 😭

Left couple of questions to discuss.

The last question: do we need to change wording in
src/plugins/discover/public/application/components/sidebar/discover_field_details.tsx for Visualize button since it has no more connections to visualize app?
the same question for tracking clicks naming:
image

I think it's ok to leave it this way, users are generally not aware to which module they are directed, so it's meant in a way like "show me something nice - visualize" .. and Lens for them isn't a separate module. furthermore they can also land in Maps.

@stratoula
Copy link
Contributor Author

Adding a few cents from Discover side

Code removing LGTM overall, a bit sad to see the Visualize sunset 😢

@sulemanof sharing your pain from Discover side, it was fun all these years 😭

Left couple of questions to discuss.
The last question: do we need to change wording in
src/plugins/discover/public/application/components/sidebar/discover_field_details.tsx for Visualize button since it has no more connections to visualize app?
the same question for tracking clicks naming:
image

I think it's ok to leave it this way, users are generally not aware to which module they are directed, so it's meant in a way like "show me something nice - visualize" .. and Lens for them isn't a separate module. furthermore they can also land in Maps.

I agree with Matthias on that

@sulemanof
Copy link
Contributor

sulemanof commented Feb 2, 2021 •
edited
Loading

sharing your pain from Discover side, it was fun all these years 😭

@kertal we could arrange an evening of memories and nostalgia for that times 😄

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
visualize 81 79 -2

Async chunks

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

id before after diff
visualize 98.4KB 98.6KB +213.0B

Page load bundle

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

id before after diff
visualize 33.6KB 28.0KB -5.6KB

History

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

kertal
kertal approved these changes Feb 3, 2021
View reviewed changes
Copy link
Member

@kertal kertal left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Code LGTM 👍 , tested locally with Chrome/Safari, running our functional test server, no Visualize Button is displayed, running our regular dev build, Visualize Button is displayed. Works as expected! Looking forward to the evening of memories and nostalgia that @sulemanof suggested, where we can share the favorite features we deprecated, the prettiest bugs we've committed or solved 🥳

@stratoula stratoula merged commit d8f7997 into elastic:master Feb 3, 2021
@stratoula stratoula mentioned this pull request Feb 3, 2021
Merged
stratoula added a commit to stratoula/kibana that referenced this pull request Feb 3, 2021
@stratoula @kibanamachine
Remove navigation from Discover to Visualize (elastic#89132)
483a6a1 
* Remove navigation from Discover to Visualize

* Remove unused translation

* Remove oss check from functional test

* Fix functional test

* Skip test for OSS

* Fix

* Should not remove the uiSettings getter and setter

* Move the isOss flag inside the test

* Cleanup

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
stratoula added a commit that referenced this pull request Feb 3, 2021
@stratoula @kibanamachine
Remove navigation from Discover to Visualize (#89132) (#90129)
cc3eb8e 
* Remove navigation from Discover to Visualize

* Remove unused translation

* Remove oss check from functional test

* Fix functional test

* Skip test for OSS

* Fix

* Should not remove the uiSettings getter and setter

* Move the isOss flag inside the test

* Cleanup

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kertal kertal kertal approved these changes

@sulemanof sulemanof sulemanof approved these changes

Assignees
No one assigned
Labels
Feature:Discover Discover Application Feature:Vis Editor Visualization editor issues release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.12.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@stratoula @elasticmachine @kibanamachine @kertal @sulemanof