Skip to content

Conversation

@pure-zero
Copy link
Contributor

#227851

Summary

Extracts IndexPatternEditor out of IndexPatternTable to avoid state changes

A full component rerender is run when onTryESQL state is updated, which causes the flyout animation to be run again

Screenshot 2025-07-21 at 10 44 41 pm

Another change could be to have the NoDataViewsPrompt not take onTryESQL as a prop. However, it is intended to be a presentational prop, so that could be a more fundamental change. I would appreciate some input on that.

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.

Identify risks

Does this PR introduce any risks? For example, consider risks like hard to test bugs, performance regression, potential of data loss.

Describe the risk, its severity, and mitigation for each identified risk. Invite stakeholders and evaluate how to proceed before merging.

@pure-zero pure-zero requested a review from a team as a code owner July 21, 2025 11:11
@pure-zero pure-zero force-pushed the double-flyout-227851 branch from 3994942 to afe824e Compare July 21, 2025 11:13
Copy link
Contributor

@davismcphee davismcphee left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution @pure-zero! Running CI for now so you can see if there are any failures.

@davismcphee
Copy link
Contributor

/ci

@davismcphee davismcphee requested a review from a team July 24, 2025 03:06
@davismcphee davismcphee added the Team:DataDiscovery Discover, search (data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. t// label Jul 24, 2025
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-data-discovery (Team:DataDiscovery)

@pure-zero
Copy link
Contributor Author

💔 Build Failed

Failed CI Steps

History

Looks like this was built on the commit previous to the auto commit can we run the ci on this again

@akowalska622
Copy link
Contributor

akowalska622 commented Jul 25, 2025

Hi @pure-zero! You're right about ESLint - the auto commit should fix this.
Although, I can spot one type check error - I think in src/platform/plugins/shared/data_view_management/public/components/index_pattern_table/index_pattern_table.tsx there's isRollup residue and it's being unused causing the TS error.

Could you double check? We can run CI again after fixing this. Thanks!

@pure-zero
Copy link
Contributor Author

Hi @pure-zero! You're right about ESLint - the auto commit should fix this. Although, I can spot one type check error - I think in src/platform/plugins/shared/data_view_management/public/components/index_pattern_table/index_pattern_table.tsx there's isRollup residue and it's being unused causing the TS error.

Could you double check? We can run CI again after fixing this. Thanks!

Thanks for that @akowalska622 I have updated that now, sorry missed that in the clean up.

@akowalska622
Copy link
Contributor

Thanks for that @akowalska622 I have updated that now, sorry missed that in the clean up.

No worries! Running the CI now

@akowalska622
Copy link
Contributor

/ci

@pure-zero
Copy link
Contributor Author

@akowalska622 Can you take another look at this? I am unable to add the required labels I believe.

@akowalska622
Copy link
Contributor

Of course, on it! Apologies for the delay

@akowalska622 akowalska622 added release_note:fix backport:all-open Backport to all branches that could still receive a release labels Aug 1, 2025
@akowalska622
Copy link
Contributor

@davismcphee could you verify backport label? Thanks!

@akowalska622
Copy link
Contributor

/ci

@davismcphee davismcphee added backport:skip This PR does not require backporting and removed backport:all-open Backport to all branches that could still receive a release labels Aug 1, 2025
@davismcphee
Copy link
Contributor

@elasticmachine run docs-build

@davismcphee
Copy link
Contributor

davismcphee commented Aug 1, 2025

@akowalska622 Where it's just a minor UI bug and low impact, I'd suggest we skip the backports for this one. The risk of introducing a regression in an older branch is probably higher than the value of the fix IMO. I updated the backport label. Also ran the docs build since we'll need it to pass before we can merge.

@akowalska622
Copy link
Contributor

Sounds good to me, thank you!

@akowalska622
Copy link
Contributor

/ci

@akowalska622
Copy link
Contributor

@elasticmachine run docs-build

@elasticmachine
Copy link
Contributor

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #43 / @ess @serverless @serverlessQA SecuritySolution Timeline @skipInServerless Timeline migrations 7.16.0 "after all" hook in "7.16.0"
  • [job] [logs] FTR Configs #43 / @ess @serverless @serverlessQA SecuritySolution Timeline @skipInServerless Timeline migrations 7.16.0 "before all" hook in "7.16.0"

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
dataViewManagement 186 187 +1

Async chunks

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

id before after diff
dataViewManagement 120.3KB 120.5KB +204.0B

History

Copy link
Contributor

@akowalska622 akowalska622 left a comment

Choose a reason for hiding this comment

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

Looks good to me. I checked it out locally and it's working as intended, thanks for the contribution!

@akowalska622 akowalska622 merged commit a0fef07 into elastic:main Aug 5, 2025
12 checks passed
@pure-zero
Copy link
Contributor Author

Thanks for the help getting that in @akowalska622 and @davismcphee

delanni pushed a commit to delanni/kibana that referenced this pull request Aug 5, 2025
…m a separate page (elastic#227851) (elastic#228749)

elastic#227851 

## Summary

Extracts IndexPatternEditor out of IndexPatternTable to avoid state
changes

A full component rerender is run when onTryESQL state is updated, which
causes the flyout animation to be run again

<img width="1364" height="733" alt="Screenshot 2025-07-21 at 10 44
41 pm"
src="https://github.com/user-attachments/assets/490392cd-4c44-4cf1-8880-8d4e93c547fc"
/>

Another change could be to have the NoDataViewsPrompt not take onTryESQL
as a prop. However, it is intended to be a presentational prop, so that
could be a more fundamental change. I would appreciate some input on
that.

### Checklist

Check the PR satisfies following conditions. 

Reviewers should verify this PR satisfies this list as well.

- [x] Any text added follows [EUI's writing
guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses
sentence case text and includes [i18n
support](https://github.com/elastic/kibana/blob/main/src/platform/packages/shared/kbn-i18n/README.md)
- [x]
[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)
was added for features that require explanation or tutorials
- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
- [x] If a plugin configuration key changed, check if it needs to be
allowlisted in the cloud and added to the [docker
list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)
- [x] 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](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was
used on any tests changed
- [x] The PR description includes the appropriate Release Notes section,
and the correct `release_note:*` label is applied per the
[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)
- [ ] Review the [backport
guidelines](https://docs.google.com/document/d/1VyN5k91e5OVumlc0Gb9RPa3h1ewuPE705nRtioPiTvY/edit?usp=sharing)
and apply applicable `backport:*` labels.
 

### Identify risks

Does this PR introduce any risks? For example, consider risks like hard
to test bugs, performance regression, potential of data loss.

Describe the risk, its severity, and mitigation for each identified
risk. Invite stakeholders and evaluate how to proceed before merging.

- [ ] [See some risk
examples](https://github.com/elastic/kibana/blob/main/RISK_MATRIX.mdx)
- [ ] ...

---------

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Ania Kowalska <63072419+akowalska622@users.noreply.github.com>
@wildemat wildemat mentioned this pull request Aug 7, 2025
10 tasks
NicholasPeretti pushed a commit to NicholasPeretti/kibana that referenced this pull request Aug 18, 2025
…m a separate page (elastic#227851) (elastic#228749)

elastic#227851 

## Summary

Extracts IndexPatternEditor out of IndexPatternTable to avoid state
changes

A full component rerender is run when onTryESQL state is updated, which
causes the flyout animation to be run again

<img width="1364" height="733" alt="Screenshot 2025-07-21 at 10 44
41 pm"
src="https://github.com/user-attachments/assets/490392cd-4c44-4cf1-8880-8d4e93c547fc"
/>

Another change could be to have the NoDataViewsPrompt not take onTryESQL
as a prop. However, it is intended to be a presentational prop, so that
could be a more fundamental change. I would appreciate some input on
that.

### Checklist

Check the PR satisfies following conditions. 

Reviewers should verify this PR satisfies this list as well.

- [x] Any text added follows [EUI's writing
guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses
sentence case text and includes [i18n
support](https://github.com/elastic/kibana/blob/main/src/platform/packages/shared/kbn-i18n/README.md)
- [x]
[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)
was added for features that require explanation or tutorials
- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
- [x] If a plugin configuration key changed, check if it needs to be
allowlisted in the cloud and added to the [docker
list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)
- [x] 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](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was
used on any tests changed
- [x] The PR description includes the appropriate Release Notes section,
and the correct `release_note:*` label is applied per the
[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)
- [ ] Review the [backport
guidelines](https://docs.google.com/document/d/1VyN5k91e5OVumlc0Gb9RPa3h1ewuPE705nRtioPiTvY/edit?usp=sharing)
and apply applicable `backport:*` labels.
 

### Identify risks

Does this PR introduce any risks? For example, consider risks like hard
to test bugs, performance regression, potential of data loss.

Describe the risk, its severity, and mitigation for each identified
risk. Invite stakeholders and evaluate how to proceed before merging.

- [ ] [See some risk
examples](https://github.com/elastic/kibana/blob/main/RISK_MATRIX.mdx)
- [ ] ...

---------

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Ania Kowalska <63072419+akowalska622@users.noreply.github.com>
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 💝community release_note:fix Team:DataDiscovery Discover, search (data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. t// v9.2.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants