Skip to content

[Streams] EditIlmPhasesFlyout and EditDslStepsFlyout UI fixes#254930

Merged
damian-polewski merged 15 commits intoelastic:mainfrom
damian-polewski:streams/downsampling_flyouts_ui_fixes
Mar 4, 2026
Merged

[Streams] EditIlmPhasesFlyout and EditDslStepsFlyout UI fixes#254930
damian-polewski merged 15 commits intoelastic:mainfrom
damian-polewski:streams/downsampling_flyouts_ui_fixes

Conversation

@damian-polewski
Copy link
Copy Markdown
Contributor

@damian-polewski damian-polewski commented Feb 25, 2026

Closes #254188
Closes #254852

Summary

This PR fixes issues with EditIlmPhasesFlyout and EditDslStepsFlyout components listed below:

How to test

View and test the component in Storybook:

yarn storybook streams_app

Checklist

Check the PR satisfies following conditions.

Reviewers should verify this PR satisfies this list as well.

  • 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
  • If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list
  • This was checked for breaking HTTP API changes, and any breaking changes have been approved by the breaking-change committee. The release_note:breaking label should be applied in these situations.
  • Flaky Test Runner was used on any tests changed
  • The PR description includes the appropriate Release Notes section, and the correct release_note:* label is applied per the guidelines
  • Review the backport guidelines and apply applicable backport:* labels.

@damian-polewski damian-polewski self-assigned this Feb 25, 2026
@damian-polewski damian-polewski added Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more t// release_note:skip Skip the PR/issue when compiling release notes backport:skip This PR does not require backporting Feature:Streams This is the label for the Streams Project labels Feb 26, 2026
@damian-polewski damian-polewski marked this pull request as ready for review February 26, 2026 11:13
@damian-polewski damian-polewski requested review from a team as code owners February 26, 2026 11:13
@elasticmachine
Copy link
Copy Markdown
Contributor

Pinging @elastic/kibana-management (Team:Kibana Management)

Copy link
Copy Markdown
Contributor

@SoniaSanzV SoniaSanzV left a comment

Choose a reason for hiding this comment

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

Everything works really well, thank you for addressing this!

Copy link
Copy Markdown
Contributor

@CoenWarmer CoenWarmer left a comment

Choose a reason for hiding this comment

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

Hi, these are my findings:

1. Naming / content mismatch for format_size_units.ts

The file is called format_size_units but the contents of the file only deal with time conversions. The naming suggests it contains other unit conversions as well (i.e. byte/kb/mb). You might want to consider naming the file format_time_units.ts to help humans / LLMs find the file faster.

2. Don't use MS_IN_SECOND * MS_IN_SECOND as a unit

I would suggest not using MS_IN_SECOND * MS_IN_SECOND as a unit, but instead make it explicit:

const MS_IN_SECOND = 1000;
const SECONDS_IN_MINUTE = 60;
const MINUTES_IN_HOUR = 60;
const HOURS_IN_DAY = 24;
const MICROS_IN_MS = 1_000;
const NANOS_IN_MS = 1_000_000;

const getMsPerUnit = (unit: string): number | undefined => {
  switch (unit) {
    case 'nanos':
      return 1 / (NANOS_IN_MS);
    case 'micros':
      return 1 / MS_IN_SECOND;
     ...
};

3. Tests

splitSizeAndUnits, getTimeUnitLabel, and isZeroAge are all exported from the source file, but none appear in the test file. Since isZeroAge in particular has non-trivial logic (guards against non-finite numbers), it warrants coverage.

Copy link
Copy Markdown
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

Most changes are small, left a comment about one part

@damian-polewski
Copy link
Copy Markdown
Contributor Author

Hey @CoenWarmer, regarding your feedback - I'm only changing the regex in format_size_units.ts and adjust tests to accept decimal values. I'm not sure if this is something we should be addressing in this PR.

Copy link
Copy Markdown
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

Looks better, while we are fixing small stuff around this flyout, another thing I noticed is that the form state is reset when the flyout switches from in-layout to overlay:

Image

Not critical, but would be good to fix

@elasticmachine
Copy link
Copy Markdown
Contributor

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] Scout: [ observability / synthetics ] plugin / local-stateful-classic - DefaultStatusAlert - creates default alert, triggers on down status, and recovers

Metrics [docs]

Async chunks

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

id before after diff
streamsApp 1.8MB 1.8MB +1.4KB

History

cc @damian-polewski

@damian-polewski
Copy link
Copy Markdown
Contributor Author

@flash1293 Good catch! This happens because EuiFlyout transition from push to overlay effectively remount parts of the flyout. hook_form_lib unregisters fields on unmount and when those fields mount again they read from the original defaultValue.

To solve this we could chose one of these solutions:

I’d suggest we handle it in a separate issue (#256018) so we don’t block this PR further.

Copy link
Copy Markdown
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM then

@damian-polewski damian-polewski merged commit a9c17ba into elastic:main Mar 4, 2026
17 checks passed
@damian-polewski damian-polewski deleted the streams/downsampling_flyouts_ui_fixes branch March 4, 2026 15:11
SoniaSanzV pushed a commit to SoniaSanzV/kibana that referenced this pull request Mar 5, 2026
…c#254930)

Closes elastic#254188
Closes elastic#254852

## Summary

This PR fixes issues with `EditIlmPhasesFlyout` and `EditDslStepsFlyout`
components listed below:

- (ILM/DSL) Remove overflow fade effect in tabs row,
- (ILM/DSL) When different tab is selected externally, scroll to the
newly selected tab,
- (ILM/DSL) Update fixed_interval help text -
elastic#253393 (comment),
- (ILM/DSL) Show help text when error is present -
elastic#253393 (comment).
- (ILM/DSL) Update copy -
elastic#254852,
rbrtj pushed a commit to rbrtj/kibana that referenced this pull request Mar 5, 2026
…c#254930)

Closes elastic#254188
Closes elastic#254852

## Summary

This PR fixes issues with `EditIlmPhasesFlyout` and `EditDslStepsFlyout`
components listed below:

- (ILM/DSL) Remove overflow fade effect in tabs row,
- (ILM/DSL) When different tab is selected externally, scroll to the
newly selected tab,
- (ILM/DSL) Update fixed_interval help text -
elastic#253393 (comment),
- (ILM/DSL) Show help text when error is present -
elastic#253393 (comment).
- (ILM/DSL) Update copy -
elastic#254852,
DennisKo pushed a commit to DennisKo/kibana that referenced this pull request Mar 6, 2026
…c#254930)

Closes elastic#254188
Closes elastic#254852

## Summary

This PR fixes issues with `EditIlmPhasesFlyout` and `EditDslStepsFlyout`
components listed below:

- (ILM/DSL) Remove overflow fade effect in tabs row,
- (ILM/DSL) When different tab is selected externally, scroll to the
newly selected tab,
- (ILM/DSL) Update fixed_interval help text -
elastic#253393 (comment),
- (ILM/DSL) Show help text when error is present -
elastic#253393 (comment).
- (ILM/DSL) Update copy -
elastic#254852,
qn895 pushed a commit to qn895/kibana that referenced this pull request Mar 11, 2026
…c#254930)

Closes elastic#254188
Closes elastic#254852

## Summary

This PR fixes issues with `EditIlmPhasesFlyout` and `EditDslStepsFlyout`
components listed below:

- (ILM/DSL) Remove overflow fade effect in tabs row,
- (ILM/DSL) When different tab is selected externally, scroll to the
newly selected tab,
- (ILM/DSL) Update fixed_interval help text -
elastic#253393 (comment),
- (ILM/DSL) Show help text when error is present -
elastic#253393 (comment).
- (ILM/DSL) Update copy -
elastic#254852,
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport:skip This PR does not require backporting Feature:Streams This is the label for the Streams Project release_note:skip Skip the PR/issue when compiling release notes Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more t// v9.4.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Downsampling] Update copies [Streams] EditIlmPhasesFlyout and EditDslStepsFlyout UI fixes

6 participants