[Controls] Decouple control fixed width and auto size settings #131769

cqliu1 · 2022-05-06T20:53:21Z

Summary

This splits up the auto size setting from the rest of the fixed width options, i.e. small, medium, and large. With the two separate sittings, setting a width and checking the switch to expand the control will result in the selected width acting as the minimum width of the control.

Create new control/edit existing control

Control group settings

~~Checking Set all widths to default will set all widths to the default width and grow setting for now until we remove it for #130702.~~

We've decided to remove all of the width settings from the control group settings. Instead of allowing the user to set their default width and expand settings, we will just cache the settings from their last created control and use those settings as the default for subsequent controls.

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

Risk Matrix

Delete this section if it is not applicable to this PR.

Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release.

When forming the risk matrix, consider some of the following examples and how they may potentially impact the change:

Risk	Probability	Severity	Mitigation/Notes
Multiple Spaces—unexpected behavior in non-default Kibana Space.	Low	High	Integration tests will verify that all features are still supported in non-default Kibana Space and when user switches between spaces.
Multiple nodes—Elasticsearch polling might have race conditions when multiple Kibana nodes are polling for the same tasks.	High	Low	Tasks are idempotent, so executing them multiple times will not result in logical error, but will degrade performance. To test for this case we add plenty of unit tests around this logic and document manual testing procedure.
Code should gracefully handle cases when feature X or plugin Y are disabled.	Medium	High	Unit tests will verify that any feature flag or plugin combination still results in our service operational.
See more potential risk examples

For maintainers

This was checked for breaking API changes and was labeled appropriately

elasticmachine · 2022-05-06T20:53:23Z

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

Heenawter

A few questions/suggestions about possible behaviour changes - specifically, whether or not creating a new control should impact the default size. Could be tackled as part of this PR, or I could do it as part of my PR for removing the "Set all sizes to default" checkbox - either is good for me :)

Heenawter · 2022-05-09T16:38:24Z

src/plugins/controls/public/control_group/editor/control_editor.tsx

@@ -84,6 +87,7 @@ export const ControlEditor = ({
  const [defaultTitle, setDefaultTitle] = useState<string>();
  const [currentTitle, setCurrentTitle] = useState(title);
  const [currentWidth, setCurrentWidth] = useState(width);
+  const [currentGrow, setCurrentGrow] = useState(true);


I think this should probably default to the defaultControlGrow rather than just true.

Suggested change

const [currentGrow, setCurrentGrow] = useState(true);

const [currentGrow, setCurrentGrow] = useState(defaultControlGrow);

Heenawter · 2022-05-09T16:51:11Z

src/plugins/controls/public/control_group/editor/create_control.tsx

@@ -83,6 +85,7 @@ export const CreateControlButton = ({
              width={defaultControlWidth ?? DEFAULT_CONTROL_WIDTH}
              updateTitle={(newTitle) => (inputToReturn.title = newTitle)}
              updateWidth={updateDefaultWidth}
+              updateGrow={updateDefaultGrow}


I don't think we necessarily want to remember this setting between controls, right? From my understanding of this, we should only remember the default control size, which is managed exclusively through the control group settings flyout. Therefore, when creating a new control, the size options should default to this - but changing them when creating a new control and/or editing an existing control should not impact the default size.

Here is a video to show what I mean when I say that creating a new control impacts the default (which, in my opinion, is a bit confusing):

2022-05-09_DecoupleBug2.mp4

I think this is actually the same behaviour we had for control width before this change, which I'm honestly surprised wasn't caught before! Perhaps @andreadelrio could provide some input here, just in case this is actually the desired behaviour :)

Edit: I took care of this as part of my PR for removing the "Set all sizes to default" checkbox (specifically in 0696a71) - here's a video of the new behaviour:

2022-05-09_ManageDefaultViaSettings.mp4

Personally, this new way of managing control sizes is much clearer to me - basically, adding a new control will not impact the default size in any way, only the "Control group settings" flyout will do that. Combined with my PR which introduces a modal that confirms whether or not the new default should be applied to existing controls when modified in the control group settings, I think it's a lot more straight forward! Obviously, this would need some discussion/agreement from everybody, though (especially @andreadelrio and @teresaalvarezsoler)

Just confirming here that the behaviour of "setting the size of the control during control creation sets the default size" was something that happened because I implemented the size check buttons and wanted to store the last used size before I implemented the default size picker in the settings menu, then forgot to remove the original behaviour. So it was indeed the default behaviour before this PR.

I agree that we should separate the two, especially because the default size is ~~saved into the dashboard saved object~~ part of the control group input. So only changing the default size via the settings should actually change it.

As we discussed in the controls sync, I've now removed all width/expand settings from the group controls since they were causing a bit of confusion.

I'm preserving the behavior that Hannah and Devon were describing where we store the selected width and expand settings on their last control and using those settings as the default when creating new controls. These settings will however revert back to the default medium width and expand true settings if you refresh or navigate away from the dashboard you were adding controls to.

andreadelrio

I think this is a cool improvement! I do have some concerns about the label of the setting because if the toggle is off users are actually setting a minimum width? so I wonder if that would lead to confusion. I think for sure we need to add some help text to this setting. I suggest we add an info icon either next to the label or next to the toggle's text (like the former will work better). There we can explain how this setting works (minimum width, how the toggle changes the behavior, etc).

Heenawter

Caught a bug that I thought belonged in main but it actually belongs here, so added a second review - sorry about that! Should have double checked 😄

Heenawter · 2022-05-10T19:17:46Z

src/plugins/controls/public/control_group/editor/control_editor.tsx

@@ -211,6 +214,23 @@ export const ControlEditor = ({
              {ControlGroupStrings.manageControl.getCancelTitle()}
            </EuiButtonEmpty>
          </EuiFlexItem>
+          <EuiFlexItem grow />
+          <EuiFlexItem grow={false}>


Talked with Andrea about this after I logged the following bug and she mentioned that you guys agreed to keep the delete button in its original place - I assume this was left in by accident? Will close the bug - didn't realize it was caused by this code, haha! Oops 😆 But yeah, we should move the "Delete" button back to its original place.

…le-size-and-stretch

cqliu1 · 2022-05-11T20:01:28Z

@andreadelrio How does this look for the info tooltip next to the Control width label? This will be visible in both the create control flyout and the edit control flyout.

@KOTungseth Any edits for the copy here?

ThomThomson · 2022-05-11T20:03:32Z

Do folks think it's worth changing the label of the Control width setting to Minimum control width when the Expand width to fit available space setting is turned on?

Heenawter · 2022-05-11T20:11:47Z

Do folks think it's worth changing the label of the Control width setting to Minimum control width when the Expand width to fit available space setting is turned on?

I like that! Then we could possibly also make the tooltip show up only when that toggle is set to true, since the text isn't applicable to when it's false 👍

andreadelrio · 2022-05-11T20:53:03Z

Do folks think it's worth changing the label of the Control width setting to Minimum control width when the Expand width to fit available space setting is turned on?

I had considered this but I don't think we should be changing the labels of fields, it would make the form not look as clean. What we could alternatively do is change the label permanently to Minimum width and then move the question icon to the right of the toggle.

…le-size-and-stretch

cqliu1 · 2022-05-16T16:40:23Z

@elasticmachine merge upstream

Heenawter

Tested it with the new changes - works great!! 👍

andreadelrio · 2022-05-16T18:49:18Z

src/plugins/controls/public/control_group/control_group.scss

@@ -54,6 +57,21 @@ $controlMinWidth: $euiSize * 14;
      cursor: grabbing;
    }
  }
+
+  @media only screen and (max-width: 767px) {


Suggested change

@media only screen and (max-width: 767px) {

@include euiBreakpoint('xs', 's', 'm') {

I believe this is the EUI way to achieve this

…le-size-and-stretch

andreadelrio

LGTM 🎉

kibana-ci · 2022-05-17T18:30:26Z

💛 Build succeeded, but was flaky

Failed CI Steps

FTR Configs #32

Test Failures

[job] [logs] FTR Configs #32 / alerting api integration spaces only Alerting bulkEdit should return mapped params after bulk edit

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
`controls`	204	207	+3

Async chunks

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

id	before	after	diff
`controls`	420.8KB	423.3KB	+2.5KB

Page load bundle

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

id	before	after	diff
`controls`	31.8KB	31.8KB	-57.0B

Unknown metric groups

API count

id	before	after	diff
`controls`	210	213	+3

History

💚 Build #45134 succeeded 75f2f07
💚 Build #45083 succeeded c22cc5e
💚 Build #45008 succeeded 5da1d6b
💚 Build #44638 succeeded f7f065e
💚 Build #44632 succeeded d6b7041
💔 Build #44244 failed dd264a9

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

cqliu1 added 3 commits May 4, 2022 12:12

Decouple control auto sizing and control width

e6efdd0

Add grow control setting to control group settings flyout

b67cc8b

Set min-width style for each control size

c18c03b

cqliu1 requested review from a team as code owners May 6, 2022 20:53

Heenawter mentioned this pull request May 9, 2022

[Controls] Remove "Set all sizes to default" checkbox in control group settings #131771

Closed

6 tasks

Heenawter reviewed May 9, 2022

View reviewed changes

andreadelrio requested changes May 9, 2022

View reviewed changes

Heenawter requested changes May 10, 2022

View reviewed changes

Heenawter mentioned this pull request May 10, 2022

[Controls] Mobile view hides save button in "Edit control" flyout #131874

Closed

cqliu1 added 3 commits May 11, 2022 09:34

Remove width settings from control group settings

4925e1d

Add media query to remove min-widths in small viewport

d8c1579

Fix grow state management

87adf6f

Heenawter mentioned this pull request May 11, 2022

[Controls] Remove default size options from control group settings #130702

Closed

Merge branch 'main' of github.com:elastic/kibana into controls/decoup…

8294258

…le-size-and-stretch

cqliu1 added 3 commits May 11, 2022 13:57

Fix i18n errors

dd264a9

Merge branch 'main' of github.com:elastic/kibana into controls/decoup…

ef92bd0

…le-size-and-stretch

Fix control group settings functional tests

d6b7041

Merge branch 'main' of github.com:elastic/kibana into controls/decoup…

f7f065e

…le-size-and-stretch

Merge branch 'main' into controls/decouple-size-and-stretch

5da1d6b

Heenawter approved these changes May 16, 2022

View reviewed changes

andreadelrio requested changes May 16, 2022

View reviewed changes

cqliu1 added 4 commits May 16, 2022 13:00

Updated control width label and move tooltip icon

c22cc5e

Use euiBreakpoints for media queries

75f2f07

Removed tooltip

8545f3c

Merge branch 'main' of github.com:elastic/kibana into controls/decoup…

774c466

…le-size-and-stretch

andreadelrio self-requested a review May 17, 2022 15:46

andreadelrio approved these changes May 17, 2022

View reviewed changes

Merge branch 'main' into controls/decouple-size-and-stretch

3dac116

cqliu1 enabled auto-merge (squash) May 17, 2022 16:15

cqliu1 disabled auto-merge May 17, 2022 16:37

Merge branch 'main' into controls/decouple-size-and-stretch

5f704ff

cqliu1 enabled auto-merge (squash) May 17, 2022 18:09

cqliu1 merged commit 6375d90 into elastic:main May 17, 2022

kibanamachine added the backport:skip This commit does not require backporting label May 17, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Controls] Decouple control fixed width and auto size settings #131769

[Controls] Decouple control fixed width and auto size settings #131769

cqliu1 commented May 6, 2022 •

edited

Loading

elasticmachine commented May 6, 2022

Heenawter left a comment •

edited

Loading

Heenawter May 9, 2022

Heenawter May 9, 2022

Heenawter May 9, 2022 •

edited

Loading

ThomThomson May 16, 2022 •

edited

Loading

cqliu1 May 16, 2022

andreadelrio left a comment

Heenawter left a comment

Heenawter May 10, 2022

cqliu1 commented May 11, 2022

ThomThomson commented May 11, 2022 •

edited

Loading

Heenawter commented May 11, 2022

andreadelrio commented May 11, 2022

cqliu1 commented May 16, 2022

Heenawter left a comment

andreadelrio May 16, 2022

andreadelrio left a comment

kibana-ci commented May 17, 2022

API count

	const [currentGrow, setCurrentGrow] = useState(true);
	const [currentGrow, setCurrentGrow] = useState(defaultControlGrow);

	@media only screen and (max-width: 767px) {
	@include euiBreakpoint('xs', 's', 'm') {

[Controls] Decouple control fixed width and auto size settings #131769

[Controls] Decouple control fixed width and auto size settings #131769

Conversation

cqliu1 commented May 6, 2022 • edited Loading

Summary

Create new control/edit existing control

Control group settings

Checklist

Risk Matrix

For maintainers

elasticmachine commented May 6, 2022

Heenawter left a comment • edited Loading

Choose a reason for hiding this comment

Heenawter May 9, 2022

Choose a reason for hiding this comment

Heenawter May 9, 2022

Choose a reason for hiding this comment

Heenawter May 9, 2022 • edited Loading

Choose a reason for hiding this comment

ThomThomson May 16, 2022 • edited Loading

Choose a reason for hiding this comment

cqliu1 May 16, 2022

Choose a reason for hiding this comment

andreadelrio left a comment

Choose a reason for hiding this comment

Heenawter left a comment

Choose a reason for hiding this comment

Heenawter May 10, 2022

Choose a reason for hiding this comment

cqliu1 commented May 11, 2022

ThomThomson commented May 11, 2022 • edited Loading

Heenawter commented May 11, 2022

andreadelrio commented May 11, 2022

cqliu1 commented May 16, 2022

Heenawter left a comment

Choose a reason for hiding this comment

andreadelrio May 16, 2022

Choose a reason for hiding this comment

andreadelrio left a comment

Choose a reason for hiding this comment

kibana-ci commented May 17, 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

cqliu1 commented May 6, 2022 •

edited

Loading

Heenawter left a comment •

edited

Loading

Heenawter May 9, 2022 •

edited

Loading

ThomThomson May 16, 2022 •

edited

Loading

ThomThomson commented May 11, 2022 •

edited

Loading