[Controls] Deprecate old controls by Heenawter · Pull Request #132562 · elastic/kibana

Heenawter · 2022-05-19T21:48:36Z

Summary

Since the new input controls are now on by default in 8.3, the old controls need to be deprecated so that users know we are moving away from them 🎉

This PR adds a new stage to visualizations - deprecated, which impacts...

the Visualize common editor:
the dropdown visualization type menu in Dashboard:

It also works with the CHARTS_TO_BE_DEPRECATED array so that a custom warning can be shown when in the editor:

Checklist

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Unit or functional tests were updated or added to match the most common scenarios
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)

For maintainers

This was checked for breaking API changes and was labeled appropriately

Heenawter · 2022-05-20T15:56:15Z

I removed these tests entirely because they no longer apply now that input controls have been moved to the new deprecated stage from the experimental stage - this was discussed with @stratoula to ensure that this was the right move, since deleting tests is always a bit scary 😆

Heenawter · 2022-05-20T16:01:01Z

Made this chartConfigToken optional because, for the deprecated input controls, there is currently no uiSetting toggle for it nor will there be a new visualization that replaces it - basically, the goal is that the input controls visualization will eventually be removed entirely rather than replaced with a newer version.

So, since the input controls don't have a chartConfigToken, I needed to make this optional so that I could still use the VizChartWarning for the input control warning.

elasticmachine · 2022-05-20T17:12:19Z

Pinging @elastic/kibana-presentation (Team:Presentation)

ThomThomson

Code only review, all of the presentation team changes LGTM! Hopefully Stratoula can check with the team and get this reviewed on Monday.

All the visuals are looking great!

Heenawter · 2022-05-20T17:24:47Z

We chose to re-name these to help distinguish between the old controls (now input controls) and the new controls (just controls).

…-ref HEAD~1..HEAD --fix'

stratoula · 2022-05-23T07:28:37Z

+  return (
+    <FormattedMessage
+      id="visualizations.controls.notificationMessage"
+      defaultMessage="Input controls are deprecated and will be removed in a future release. Use the new Controls to filter and interact with your dashboard data. "


Is there any documentation for the new controls to show to the users? This is your call of course but I think, it would be better to have a link to a documentation for the new controls. Where can I find them? etc

@KOTungseth Do we have a link for the new controls docs yet? If not, I think we could safely add this after feature freeze as well as long as we can get the actual deprecation code in first :)

stratoula · 2022-05-23T12:05:50Z

@Heenawter this looks good from the code perspective. I also tested it and looks fine!

@flash1293 the presentation team wants to merge it before the FF but the thing we wanted to discuss is the following:

The presentation team wants to deprecate the old controls in favor of the new ones. The old controls were on experimental stage and now the team has introduced a new stage (deprecated). For the experimental viz types there is this switch

that the users can switch off, if they dont want these types to be used in their kibana instance.
The problem that occurs is that by changing the state from experimental to deprecated now all the users will see the input controls option both in the wizard (both visualize and dashboard wizards).

From my point of view, this is not a big problem. I think that the current PR is fine but I also need your feedback here. I discussed it also with @ghudgins and he also feels the same.

The other idea from the presentation team was to introduce a new switch (Enable deprecated visualizations) that it will affect the deprecated vis types but we find it a bit confusing for the following reasons:

We can't backport the Enable technical preview switch status to the new switch. Which means that even if the users are having this switch off and they dont want to see the input controls they have to go to the advanced settings and close the new switch
We also have the deprecated implementation of the pie and timelion. How this switch is going to work? Hiding the input controls but keeping the deprecation messages of our vislib implementations? This will be confusing for the users.

With that being said, I vote for merging this PR as it is. Wdyt?

flash1293 · 2022-05-23T13:05:03Z

What about having experimental and deprecated as two separate state properties? I don’t want to cause more work than necessary, so if it’s very hard to do I’m fine with the current approach, too - separating experimental and deprecated however feels like the cleanest solution.

stratoula · 2022-05-23T14:27:22Z

I see, this is not a bad idea actually. So the input controls stay on the experimental stage and follows the rules of this flag and is also deprecated. I like it!
@Heenawter I don't think that this will be a huge change, you will need to remove the deprecated from the stage and add a new property on the vis types something like isDeprecated. So you will use this property for all your checks.
With this way, the setting for the experimental will still work for the input controls :)

stratoula · 2022-05-23T15:23:36Z

I pushed a commit to implement what we are proposing. How do you feel about it?

Something that I noticed that is irrelevant with this PR and also reproducable at least on 8.2 is the following.

I close the enableLabs setting, go to dashboard. First thing I see the Controls as an option while I shouldn't. When I click it, I see this error.

I think that on the Select type menu, you should also check the status of this setting and not display the controls if the setting is off. I can create an issue for that

Heenawter · 2022-05-24T14:52:32Z

@stratoula Thanks so much for making the necessary changes! And for finding that bug - I wonder how long that has been around? 😆 Definitely need to fix that

@flash1293 I tested it locally, and I really like this change - keeping the old controls as experimental so the existing behaviour doesn't change, but marking them as deprecated so that users know that they will soon be removed. Awesome suggestion! :)

KOTungseth · 2022-05-24T15:12:42Z

Can we change the wording here to:
When enabled, allows you to create, view, and edit visualizations that are in technical preview. When disabled, only production-ready visualizations are available.

Heenawter · 2022-05-24T15:26:35Z

@KOTungseth

KOTungseth · 2022-05-24T16:06:41Z

Can we match the technical preview message to this:

Heenawter · 2022-05-24T16:12:02Z

@KOTungseth That technical preview text is used throughout many apps so, if we wanted to change the wording, I think it would be best to do it in a separate PR since it would require many more approvals to keep it consistent. Here's an example of it in Machine Learning:

KOTungseth · 2022-05-24T16:15:02Z

@Heenawter can we leave the words as-is, but render the text in a different color to differentiate between the regular text and link text? I like the title too, but if we can't mess with the formatting, that's cool :)

Heenawter · 2022-05-24T16:30:31Z

@KOTungseth Ahhh gotchya! Yeah, we can do that. So, when the old input controls are not marked as deprecated (since they are currently our only experimental vis to test on):

But when they are marked as deprecated:

flash1293

LGTM, works as expected.

flash1293 · 2022-05-24T17:40:50Z

      </EuiFlexItem>
      <EuiFlexItem>
-        <EuiFlexGroup gutterSize="xs" alignItems="center" responsive={false}>
+        <EuiFlexGroup gutterSize="s" alignItems="center" responsive={false}>


why this change?

Thanks so much for the review despite the fact that you are off for the day, @flash1293! It's greatly appreciated ❤️

This change was so that the visualization name and the new "Deprecated" tag had a tiny bit more space between them, like so:

kibana-ci · 2022-05-24T17:53:45Z

💛 Build succeeded, but was flaky

Failed CI Steps

FTR Configs #27

Test Failures

[job] [logs] FTR Configs #27 / rules security and spaces enabled: basic ruleRegistryAlertsSearchStrategy logs should return alerts from log rules

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`visualizations`	351	353	+2

Async chunks

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

id	before	after	diff
`dashboard`	304.4KB	304.7KB	+302.0B
`visualizations`	302.9KB	303.6KB	+778.0B
total			+1.1KB

Page load bundle

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

id	before	after	diff
`inputControlVis`	5.5KB	5.5KB	+37.0B
`visualizations`	46.5KB	46.6KB	+93.0B
total			+130.0B

Unknown metric groups

API count

id	before	after	diff
`visualizations`	372	375	+3

History

💚 Build #46973 succeeded 6c96791
💚 Build #46746 succeeded 8636769
💚 Build #46651 succeeded bf73bfa
💛 Build #46598 was flaky 1e5c5e7
💛 Build #46552 was flaky 03aa016f6c0b1b2cc18d4e6416c46f135f8dec19
💔 Build #46301 failed 64874e04e9b6ef7956e192fb7c3d59f752fc94c7

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

cc @Heenawter

* Deprecate old controls * Use badge instead of changing the name * Fix warning message * Add i18n support for deprecated tag * Fix functional and jest tests * [CI] Auto-commit changed files from 'node scripts/precommit_hook.js --ref HEAD~1..HEAD --fix' * Add an isDeprecated flag to the visTypes * Fix wording of enable labs setting * Stylize technical preview message Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Stratoula Kalafateli <efstratia.kalafateli@elastic.co>

Heenawter requested review from a team as code owners May 19, 2022 21:48

Heenawter marked this pull request as draft May 19, 2022 21:48

Heenawter added v8.3.0 release_note:deprecation labels May 19, 2022

Heenawter force-pushed the deprecate-old-controls_2022-05-19 branch 2 times, most recently from 5178bc1 to 507fa0b Compare May 19, 2022 22:40

Heenawter self-assigned this May 20, 2022

Heenawter force-pushed the deprecate-old-controls_2022-05-19 branch from 64874e0 to c96f753 Compare May 20, 2022 15:52

Heenawter commented May 20, 2022

View reviewed changes

Heenawter marked this pull request as ready for review May 20, 2022 17:12

Heenawter added the backport:skip This PR does not require backporting label May 20, 2022

ThomThomson approved these changes May 20, 2022

View reviewed changes

Heenawter commented May 20, 2022

View reviewed changes

Heenawter and others added 6 commits May 20, 2022 11:52

Deprecate old controls

9f7363e

Use badge instead of changing the name

74678ca

Fix warning message

f3f147f

Add i18n support for deprecated tag

f1ac467

Fix functional and jest tests

83c9b07

[CI] Auto-commit changed files from 'node scripts/precommit_hook.js -…

1e5c5e7

…-ref HEAD~1..HEAD --fix'

Heenawter force-pushed the deprecate-old-controls_2022-05-19 branch from 03aa016 to 1e5c5e7 Compare May 20, 2022 17:53

Heenawter and others added 2 commits May 20, 2022 15:14

Merge branch 'main' into deprecate-old-controls_2022-05-19

bf73bfa

Merge branch 'main' into deprecate-old-controls_2022-05-19

8636769

stratoula reviewed May 23, 2022

View reviewed changes

Add an isDeprecated flag to the visTypes

6c96791

Fix wording of enable labs setting

26a03bc

Stylize technical preview message

da3a028

Heenawter force-pushed the deprecate-old-controls_2022-05-19 branch from 3be0083 to da3a028 Compare May 24, 2022 16:43

flash1293 approved these changes May 24, 2022

View reviewed changes

Heenawter merged commit dbeee7a into elastic:main May 24, 2022

Conversation

Heenawter commented May 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

For maintainers

Uh oh!

Heenawter May 20, 2022

Choose a reason for hiding this comment

Uh oh!

Heenawter May 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented May 20, 2022

Uh oh!

ThomThomson left a comment

Choose a reason for hiding this comment

Uh oh!

Heenawter May 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stratoula May 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Heenawter May 24, 2022

Choose a reason for hiding this comment

Uh oh!

stratoula commented May 23, 2022

Uh oh!

flash1293 commented May 23, 2022

Uh oh!

stratoula commented May 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stratoula commented May 23, 2022

Uh oh!

Heenawter commented May 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KOTungseth commented May 24, 2022

Uh oh!

Heenawter commented May 24, 2022

Uh oh!

KOTungseth commented May 24, 2022

Uh oh!

Heenawter commented May 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KOTungseth commented May 24, 2022

Uh oh!

Heenawter commented May 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flash1293 left a comment

Choose a reason for hiding this comment

Uh oh!

flash1293 May 24, 2022

Choose a reason for hiding this comment

Uh oh!

Heenawter May 24, 2022

Choose a reason for hiding this comment

Uh oh!

kibana-ci commented May 24, 2022

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

Metrics [docs]

Public APIs missing comments

Async chunks

Page load bundle

API count

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Heenawter commented May 19, 2022 •

edited

Loading

Heenawter May 20, 2022 •

edited

Loading

Heenawter May 20, 2022 •

edited

Loading

stratoula May 23, 2022 •

edited

Loading

stratoula commented May 23, 2022 •

edited

Loading

Heenawter commented May 24, 2022 •

edited

Loading

Heenawter commented May 24, 2022 •

edited

Loading

Heenawter commented May 24, 2022 •

edited

Loading