[ILM] Delete phase redesign #89451

yuliacech · 2021-01-27T17:19:02Z

Summary

Continues the ILM re-design implementation work started in #88671

This PR implements the Delete phase and timing blocks for other phases.

Design changes:

Delete phase design changes
Bottom blocks on other phases with timing
Complete style of active highlight

Code changes:

Timing footers provider
scss for active highlight
infinity icon extracted to a separate file

Screenshots

Active highlight and phase bottom blocks

Delete phase redesign

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
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)

elasticmachine · 2021-01-28T09:59:06Z

Pinging @elastic/es-ui (Team:Elasticsearch UI)

yuliacech · 2021-01-28T14:41:12Z

@elasticmachine merge upstream

mdefazio · 2021-01-28T13:23:13Z

...nt/public/application/sections/edit_policy/components/active_highlight/active_highlight.scss

+  .ilmActivePhaseHighlight__line {
+    height: 100%;
+    position: absolute;
+    border-left: $euiBorderWidthThin solid $euiColorLightShade;


I think we can simply use border-left: $euiBorderThin here.

mdefazio · 2021-01-28T13:42:49Z

...nt/public/application/sections/edit_policy/components/active_highlight/active_highlight.scss

+    left: calc((#{$euiSize} - #{$euiBorderWidthThin}) / -2);
+    height: $euiSize;
+    width: $euiSize;
+    border: $euiBorderWidthThin solid $euiColorLightShade;


Again border: $euiBorderThin

mdefazio · 2021-01-28T13:44:13Z

...nt/public/application/sections/edit_policy/components/active_highlight/active_highlight.scss

+
+  .ilmActivePhaseHighlight__check {
+    position: absolute;
+    top: calc(#{$euiSize} * 1.5);


I believe this is the same as $euiSizeL and so we can remove the calc() function

mdefazio · 2021-01-28T13:44:48Z

...nt/public/application/sections/edit_policy/components/active_highlight/active_highlight.scss

+  &.hotPhase {
+    &.active {
+      .ilmActivePhaseHighlight__line {
+        top: calc(#{$euiSize} * 1.5);


Again here, perhaps use simply $euiSizeL

yuliacech · 2021-01-28T16:53:54Z

@elasticmachine merge upstream

yuliacech · 2021-01-28T16:55:04Z

Thanks for your review, @mdefazio ! I added your suggestions to the code, would you please have another look?

cjcenizal · 2021-01-28T22:57:36Z

Hi @yuliacech this is great progress! I reviewed this and I have some UX feedback, some of which may fall outside the scope of this PR. Please let me know which items you'd like to address separately and I'll move them into a new issue. @mdefazio I'd appreciate hearing your thoughts on this feedback too.

Timeline delete icon looks like a button

I tried to click on this, and I worry that others will too. It offers very similar affordances to the "Delete" icon buttons we have throughout our UIs. As a solution, I'd suggest replacing this with another horizontal strip with the label "Delete" below it.

Delete phase background color makes it look like a callout

This background color also causes the child callout to lose its contrast with the containing element. It seems a bit lost in there. As a solution, I'd suggest using a standard panel to contain this phase, same as the other phases.

"Set delete age" button UX lacks feedback

When I click this button:

It's replaced by this:

My first point of confusion is around the wording "Set delete age". What does that mean? It doesn't leverage the terms I use in my head when thinking about deleting data after a phase.

My second pain point was after I clicked it, it disappeared and was replaced by a message about timing. "Data remains in this phase for less than a day." Hm. What happened to the delete stuff? Can I change my mind and go back? The delete phase was added to the bottom of the form, but that was offscreen for me, so seeing stuff disappear when I clicked the button was jarring. It felt irreversible and I didn't know what I had done.

I suggest massaging the copy and the "add delete" / "remove delete" UX. Here's an idea of what I mean.

We can change the label to be "Delete data after this phase" or something like this. This tells me explicitly what I'm opting into.

Then when it's clicked, let's show the user a button for removing the delete phase. This way it's trivial to undo the action. We've already taught the user to look here for adding the delete phase and it seems logical that they'd want to look here to remove it. This label's copy could use work, but something like "Keep data in this phase" gets the idea across.

Putting the action in the last enabled phase section will allow us to remove the "Remove" button from the top right of the delete phase section, which has the nice side effect of aligning the timing fields with the timing fields of the other phases.

a11y issues

It looks like the aria-label for the toggles still says "Activate" after they've been activated. It should change to be "Deactivate", so that people using screen readers can predict what will happen when they click it. I suggest auditing the UI thoroughly to be sure all controls communicate their states accurately in a similar manner.

Responsiveness issues

You mentioned this to me earlier so I'm just noting this for completeness. When the screen is narrower, controls in the top right of the phase section wrap uncomfortably, breaking the layout. The phase name and toggle also get pushed down.

I'd suggest replacing the "Settings" button with an EuiAccordion pattern. This will also improve the feedback because it will be clear from the state of the accordion caret whether it's open or closed. This "Settings" button doesn't visually communicate whether the settings are open or not -- you have to have the settings area visible on-screen in order to tell.

Another solution would be to move it into the body of the section, below the phase description, and adjust the copy to explicitly state "Show settings" and "Hide settings". This is what we do in the mappings editor.

yuliacech · 2021-01-29T09:47:57Z

@elasticmachine merge upstream

…row)

jloleysens

@yuliacech great work! I read through the code and left some comments, none of which are blocking.

I am aware some things are still changing based on feedback, but I just wanted to share my thoughts based on the current state:

I wonder if it would be better to not have this copy in a callout, I feel like the delete phase being in red already draws a lot of attention, the extra callout seems like we are really shouting at the user 😄, perhaps we should reserve the callouts for things that we specifically want to emphasise in the delete phase block like non-existent snapshot policies. I suggest keeping that text in line with other description text we have.

I'd like to add that I think CJ's suggestion regarding the "Set delete age" in the grey blocks sounds like a change that would make this great UI even better. The ability to "Keep data" right after enabling the delete phase seems like a solid way to keep users feeling in control.

jloleysens · 2021-01-29T11:25:20Z

...nt/public/application/sections/edit_policy/components/active_highlight/active_highlight.scss

  height: 100%;

-  &.hotPhase.active {
-    border-left-color: $euiColorVis9_behindText;
+  .ilmActivePhaseHighlight__line {


Just a question on this pattern I see used fairly consistently, is there a reason we are not using &__line here? Not a blocker, perhaps something someone on the design team may have input on @cchaos

jloleysens · 2021-01-29T11:30:15Z

...ugins/index_lifecycle_management/public/application/sections/edit_policy/components/index.ts

@@ -13,5 +12,6 @@ export { DescribedFormRow, ToggleFieldWithDescribedFormRow } from './described_f
 export { FieldLoadingError } from './field_loading_error';
 export { ActiveHighlight } from './active_highlight';
 export { Timeline } from './timeline';
+export { TimingFootersProvider, useTimingFooters } from './timing_footers';


nit; I know naming is quite tough here, but I wonder if these would be improved by introducing the word "phase" here. Perhaps "PhaseFooterProvider" ("usePhaseFooter") or "PhaseTimingProvider" ("usePhaseTiming"). Let me know what you think!

jloleysens · 2021-01-29T11:32:33Z

...application/sections/edit_policy/components/phases/shared_fields/snapshot_policies_field.tsx

-                  ...field,
-                  value: singleSelectionArray,
-                } as any
+            <EuiFormRow


This looks like it is a pre-exising issue, but there may be a double EuiFormRow here (this one and ComboBoxField below). Maybe not worth addressing here though.

jloleysens · 2021-01-29T12:24:15Z

...public/application/sections/edit_policy/components/timing_footers/timing_footers_context.tsx

+  const phaseTimings = useMemo(() => {
+    return calculateRelativeTimingMs(formData);
+  }, [formData]);
+
+  const humanReadableTimings = useMemo(() => normalizeTimingsToHumanReadable(phaseTimings), [
+    phaseTimings,
+  ]);


Currently these functions (calculateRelativeTimingMs, and normalizeTimingsToHumanReadable) are being run twice, once here and once in the timeline.tsx component - for the same data. I wonder if it might be worth creating a separate context for this where all components can share the output of these computations. No need to action any of this here, we can do it as a follow up PR. I have some ongoing work to refactor some of the timeline component and timing computation logic: #89422

jloleysens · 2021-01-29T12:25:21Z

...public/application/sections/edit_policy/components/timing_footers/timing_footers_context.tsx

+  );
+
+  return (
+    <UseField path={'_meta.delete.enabled'}>


Nit; for here and elsewhere; we usually write static string props using this pattern:

<UseField path="_meta.delete.enabled">

myasonik · 2021-01-29T16:33:26Z

Any chance you can proactively add a11y tests to this? (Docs)

Will save you the trouble of having to go back and do this when QA goes through it later and makes a follow up ticket for you but you've forgotten all the context around your work. (Meta with many examples)

ILM tests are already started too!

yuliacech · 2021-02-01T11:13:06Z

@elasticmachine merge upstream

yuliacech · 2021-02-02T13:13:39Z

@elasticmachine merge upstream

ryankeairns · 2021-02-02T18:47:18Z

@yuliacech and @mdefazio this is exceptional work. I really believe this form design sets the bar, well done.

In a quick pass, I switched to the v8 theme and noticed what appears to be an empty panel:

yuliacech · 2021-02-04T13:51:29Z

@elasticmachine merge upstream

kibanamachine · 2021-02-04T13:51:31Z

merge conflict between base and head

yuliacech · 2021-02-04T14:45:09Z

sorry about the ping, team! Struggling with merge conflicts, will close this PR and open a new one for ILM re-design.

kibanamachine · 2021-02-04T15:23:15Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: 9cff1a9
Pipeline Steps (look for red circles / failed steps)
Interpreting CI Failures
Documentation Changes

Failed CI Steps

Metrics [docs]

‼️ ERROR: metrics for 9cff1a9 were not reported

History

💔 Build #103769 failed 180a0c7
💚 Build #102974 succeeded 52725f8
💚 Build #102651 succeeded 4e92ff6
💔 Build #102606 failed 34bd046

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

Delete phase re-design

aa168f1

yuliacech added Feature:ILM release_note:enhancement Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more v7.12.0 v8.0.0 labels Jan 27, 2021

Fixed i18n files

35c2fc0

yuliacech marked this pull request as ready for review January 28, 2021 09:59

yuliacech requested review from a team as code owners January 28, 2021 09:59

yuliacech requested review from jloleysens and mdefazio January 28, 2021 09:59

Merge branch 'master' into ilm_delete_phase

d4eb21d

mdefazio requested changes Jan 28, 2021

View reviewed changes

Added scss review suggestions

134355b

Merge branch 'master' into ilm_delete_phase

c068bf3

kibanamachine and others added 3 commits January 29, 2021 04:48

Merge branch 'master' into ilm_delete_phase

8ddbd10

Fix a11y id issue (snapshot policies field doesn't need another form …

33f3f54

…row)

Trying out 'danger' color for Delete phase

51b0897

jloleysens reviewed Jan 29, 2021

View reviewed changes

Merge branch 'master' into ilm_delete_phase

ba77d60

Merge branch 'master' into ilm_delete_phase

d13daa5

yuliacech added 4 commits February 1, 2021 15:46

Changed delete phase style

b412923

Fixed i18n files

34bd046

Fixed eslint

4e92ff6

Changed phase footers

42fcf64

kibanamachine and others added 2 commits February 2, 2021 08:13

Merge branch 'master' into ilm_delete_phase

a0867bc

Changes footer colors

52725f8

Added new design

180a0c7

Merge conflicts

9cff1a9

yuliacech requested review from vigneshshanmugam and a team as code owners February 4, 2021 14:37

yuliacech requested a review from a team February 4, 2021 14:37

yuliacech requested review from a team as code owners February 4, 2021 14:37

yuliacech closed this Feb 4, 2021

yuliacech mentioned this pull request Feb 4, 2021

[ILM] Delete phase redesign (rework) #90291

Merged

5 tasks

yuliacech added release_note:skip Skip the PR/issue when compiling release notes and removed release_note:enhancement labels Feb 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ILM] Delete phase redesign #89451

[ILM] Delete phase redesign #89451

yuliacech commented Jan 27, 2021 •

edited

Loading

elasticmachine commented Jan 28, 2021

yuliacech commented Jan 28, 2021

mdefazio Jan 28, 2021

mdefazio Jan 28, 2021

mdefazio Jan 28, 2021

mdefazio Jan 28, 2021

yuliacech commented Jan 28, 2021

yuliacech commented Jan 28, 2021

cjcenizal commented Jan 28, 2021 •

edited

Loading

yuliacech commented Jan 29, 2021

jloleysens left a comment •

edited

Loading

jloleysens Jan 29, 2021

jloleysens Jan 29, 2021

jloleysens Jan 29, 2021

jloleysens Jan 29, 2021

jloleysens Jan 29, 2021

myasonik commented Jan 29, 2021

yuliacech commented Feb 1, 2021

yuliacech commented Feb 2, 2021

ryankeairns commented Feb 2, 2021

yuliacech commented Feb 4, 2021

kibanamachine commented Feb 4, 2021

yuliacech commented Feb 4, 2021

kibanamachine commented Feb 4, 2021 •

edited

Loading

[ILM] Delete phase redesign #89451

[ILM] Delete phase redesign #89451

Conversation

yuliacech commented Jan 27, 2021 • edited Loading

Summary

Screenshots

Checklist

elasticmachine commented Jan 28, 2021

yuliacech commented Jan 28, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuliacech commented Jan 28, 2021

yuliacech commented Jan 28, 2021

cjcenizal commented Jan 28, 2021 • edited Loading

Timeline delete icon looks like a button

Delete phase background color makes it look like a callout

"Set delete age" button UX lacks feedback

a11y issues

Responsiveness issues

yuliacech commented Jan 29, 2021

jloleysens left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

myasonik commented Jan 29, 2021

yuliacech commented Feb 1, 2021

yuliacech commented Feb 2, 2021

ryankeairns commented Feb 2, 2021

yuliacech commented Feb 4, 2021

kibanamachine commented Feb 4, 2021

yuliacech commented Feb 4, 2021

kibanamachine commented Feb 4, 2021 • edited Loading

💔 Build Failed

Failed CI Steps

Metrics [docs]

History

yuliacech commented Jan 27, 2021 •

edited

Loading

cjcenizal commented Jan 28, 2021 •

edited

Loading

jloleysens left a comment •

edited

Loading

kibanamachine commented Feb 4, 2021 •

edited

Loading