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

Remove lab stage for visualizations #25702

Merged
merged 4 commits into from
Nov 19, 2018
Merged

Conversation

timroes
Copy link
Contributor

@timroes timroes commented Nov 15, 2018

Summary

This PR removes the lab stage of visualizations who was initially only introduced so that TSVB can live in it's own experimental stage, that can't be disabled via the advanced settings.

This PR now removes lab as a stage and instead makes experimental the stage, that can be disabled via advanced settings. We don't change the setting id (but the description and name), so existing settings, provisioning scripts, etc. won't break.

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

For maintainers

Dev Docs

Removal of lab stage for visualizations

We removed the lab stage for visualizations. The experimental stage is now the only non production stage available, and the one affected by the advanced setting to disable experimental visualizations.

Please just change a stage: 'lab' in your custom vis type to stage: 'experimental', to mark it as non production-ready/experimental.

@timroes timroes added Feature:Visualizations Generic visualization features (in case no more specific feature label is available) v7.0.0 release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. Team:Visualizations Visualization editors, elastic-charts and infrastructure v6.6.0 labels Nov 15, 2018
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app

@elasticmachine
Copy link
Contributor

💔 Build Failed

@timroes
Copy link
Contributor Author

timroes commented Nov 15, 2018

Jenkins, test this

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Member

@lukeelmers lukeelmers left a comment

Choose a reason for hiding this comment

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

overall code LGTM, just had two small notes

@@ -50,23 +50,6 @@ export default ({ getService, getPageObjects }) => {
expect(await info.getVisibleText()).to.contain('experimental');
});

it('should show an notification when creating lab visualizations', async () => {
// Try to find a lab visualization.
const labTypes = await PageObjects.visualize.getLabTypeLinks();

This comment was marked as resolved.

src/core_plugins/kibana/ui_setting_defaults.js Outdated Show resolved Hide resolved
lukeelmers and others added 2 commits November 16, 2018 09:46
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Member

@lukeelmers lukeelmers left a comment

Choose a reason for hiding this comment

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

🚢

Copy link
Member

@markov00 markov00 left a comment

Choose a reason for hiding this comment

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

Code LGTM. Also checked with Tim that the saved-object-finder directive would work correctly using saved objects from previous versions: the type.stage field is applied after loading the saved object so no conflict between lab and experimental happens here.
I've left just a minor minor minor detail comment :)

if (type.stage === 'lab') {
prefix = '(Lab)';
} else if (type.stage === 'experimental') {
if (type.stage === 'experimental') {
prefix = '(Experimental)';
}
return `${prefix} ${type.description}`;
Copy link
Member

Choose a reason for hiding this comment

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

This will always have an empty space if there is no prefix.
What about writing something like this:

if(type.stage === 'experimental'){
  return `(Experimental) ${type.description}`;
} else {
  return type.description;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will ignore your comment for now, since all of that code is already removed in #23833 :-)

@timroes timroes merged commit a5e096e into elastic:master Nov 19, 2018
@timroes timroes deleted the remove-lab-vis branch November 19, 2018 15:28
timroes added a commit to timroes/kibana that referenced this pull request Nov 19, 2018
* Remove lab stage for visualizations

* Fix typo

Co-Authored-By: timroes <[email protected]>

* Remove dead code
timroes added a commit that referenced this pull request Nov 19, 2018
* Remove lab stage for visualizations

* Fix typo

Co-Authored-By: timroes <[email protected]>

* Remove dead code
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Visualizations Generic visualization features (in case no more specific feature label is available) release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. Team:Visualizations Visualization editors, elastic-charts and infrastructure v6.6.0 v7.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants