Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DataView] show empty matching sources when no matched index pattern #195537

Merged
merged 14 commits into from
Oct 24, 2024

Conversation

lsq645599166
Copy link
Contributor

@lsq645599166 lsq645599166 commented Oct 9, 2024

Summary

Closes #194736

Show empty matching sources when no matched index pattern in create data view panel

Before

image

After

image

@lsq645599166 lsq645599166 requested a review from a team as a code owner October 9, 2024 07:32
@kertal kertal added the Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. label Oct 9, 2024
@elasticmachine
Copy link
Contributor

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

@kertal kertal added backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) and removed backport:skip This commit does not require backporting labels Oct 9, 2024
@kertal
Copy link
Member

kertal commented Oct 10, 2024

/ci

@kertal kertal self-requested a review October 11, 2024 08:10
Copy link
Member

@kertal kertal left a comment

Choose a reason for hiding this comment

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

Thx so much for this contribution 👍
To move bring this over the finishing line, there's one mandatory change:
The one Jest test failure needs to be fixed

One optional change you could try is automatically show "All sources" for the given case, because matching sources is giving no useful information anyway. But this is not mandatory, just wanted to share some thoughts

We definitely appreciate you contribution

@lsq645599166
Copy link
Contributor Author

Thx so much for this contribution 👍 To move bring this over the finishing line, there's one mandatory change: The one Jest test failure needs to be fixed

One optional change you could try is automatically show "All sources" for the given case, because matching sources is giving no useful information anyway. But this is not mandatory, just wanted to share some thoughts

We definitely appreciate you contribution

Thanks for you suggestion,I‘ll try to optimize the UX in the later commit

@lsq645599166
Copy link
Contributor Author

@kertal I have already optimize the UX and fix the unit test, could you please rerun the CI.

@lsq645599166
Copy link
Contributor Author

@elasticmachine merge upstream

@kertal
Copy link
Member

kertal commented Oct 14, 2024

sure, thx!

@kertal
Copy link
Member

kertal commented Oct 14, 2024

/ci

@kertal
Copy link
Member

kertal commented Oct 16, 2024

@elasticmachine merge upstream

@kertal
Copy link
Member

kertal commented Oct 16, 2024

Sorry that it took so long @lsq645599166 ... So those test are fixed, this is great. I was testing your latest addtion and was thinking what if we could change it so the state we aim to approve is displayed this way, no tabs are being displayed, just the list

CleanShot 2024-10-16 at 12 35 48

Wdyt @jughosta , I see you have been working on this part of the code a while ago

For me it seems the following changes would enable this, and we would need no useEffect here

 let currentlyVisibleIndices;
  let currentViewMode;

  if (
    (title.length &&
      matched.visibleIndices.length &&
      !isAboutToIncludeMoreIndices(title) &&
      viewMode !== ViewMode.allIndices) ||
    viewMode === ViewMode.onlyMatchingIndices
  ) {
    currentlyVisibleIndices = matched.visibleIndices;
    currentViewMode = ViewMode.onlyMatchingIndices;
  } else {
    currentlyVisibleIndices = matched.allIndices;
    currentViewMode = ViewMode.allIndices;
  }

and

 {Boolean(title) && Boolean(matched.visibleIndices.length) && (

wdyt @jughosta? 🙏

@jughosta
Copy link
Contributor

Sorry, don't have time to look into it now. To me it would be better to avoid too many UI changes when a user is typing a value. Otherwise it would be distracting. I rather keep the tabs.

@kertal
Copy link
Member

kertal commented Oct 17, 2024

To me it would be better to avoid too many UI changes when a user is typing a value. Otherwise it would be distracting. I rather keep the tabs.

thx @jughosta , I've tested it and you're right abot the too many UI changes. Let's focus on the problem we solve here and keep the behavior, not changing the user selection with a useEffect. So pls, @lsq645599166 can you remove the useEffect logic? Many thx!

@davismcphee
Copy link
Contributor

++ to avoiding messing with the tabs. I see how it could be helpful to auto switch tabs, but if the user makes a typo which causes it to switch, then backspaces to fix the typo, they'd now be on the "All sources" tab and have to manually switch back (trying to auto switch back again would likely be tricky and error prone). At most I'd add a "No matching sources found" message to the "Matching sources" tab to avoid an empty display (even this may not be necessary with the existing callout though, would just help reinforce and fill the empty space).

@lsq645599166
Copy link
Contributor Author

++ to avoiding messing with the tabs. I see how it could be helpful to auto switch tabs, but if the user makes a typo which causes it to switch, then backspaces to fix the typo, they'd now be on the "All sources" tab and have to manually switch back (trying to auto switch back again would likely be tricky and error prone). At most I'd add a "No matching sources found" message to the "Matching sources" tab to avoid an empty display (even this may not be necessary with the existing callout though, would just help reinforce and fill the empty space).

Thanks for suggestion, I've added a message when no matched index, reuse the i18n key indexPatternEditor.status.notMatchLabel.notMatchNoIndicesDetail
image

@lsq645599166
Copy link
Contributor Author

@elasticmachine merge upstream

@kertal
Copy link
Member

kertal commented Oct 18, 2024

/ci

@@ -86,7 +90,7 @@ export const PreviewPanel = ({ type, allowHidden, title = '', matchedIndices$ }:
query={title}
/>
<EuiSpacer size="m" />
{Boolean(title) && currentlyVisibleIndices.length > 0 && (
{Boolean(title) && (
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks again for the contribution @lsq645599166, and apologies for all the back and forth on this. After testing this locally, I think misunderstood the intent of the code originally. I also overlooked @kertal's second comment where he suggested hiding the tabs entirely, but I agree this is actually the best approach.

We already do this before the user has typed anything, and we show the full list of sources:
image

We also intentionally return the full list as visibleIndices from getMatchedIndices when there are no matches because we want the user to see the available options when their pattern doesn't match. It turns out this was behaving as intended, but the confusing part was that the tabs were still visible and "Matching sources" was still selected.

If we hide the tabs when there are no matches as @kertal suggested, it behaves the same as if the user hadn't typed anything, so there's no confusion about the tabs, and they'll still be able to see the available sources:
image

This seems like the least confusing and most consistent behaviour (and matches the original intent). Luckily if we follow the same logic used in src/plugins/data_view_editor/public/components/preview_panel/status_message/status_message.tsx, I think we should be able to revert all changes except for this line in PreviewPanel.

Suggested change
{Boolean(title) && (
{title.length > 0 && (matched.exactMatchedIndices.length > 0 || matched.partialMatchedIndices.length > 0) && (

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I commit a change to hide tab when no matched index.
image

@lsq645599166
Copy link
Contributor Author

@elasticmachine merge upstream

@davismcphee
Copy link
Contributor

/ci

@lsq645599166
Copy link
Contributor Author

@kertal @davismcphee What should I do next

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.

Hey @lsq645599166, it looks like there are still changes to getMatchedIndices. Unless I'm overlooking something, I think we should be good to revert all the changes and just handle this in the conditional for the EuiButtonGroup, as mentioned in my last comment.

@lsq645599166
Copy link
Contributor Author

Hey @lsq645599166, it looks like there are still changes to getMatchedIndices. Unless I'm overlooking something, I think we should be good to revert all the changes and just handle this in the conditional for the EuiButtonGroup, as mentioned in my last comment.

Sorry, I misunderstand the meaning of your last comment. I revert getMatchedIndices in the latest commit.

@lsq645599166
Copy link
Contributor Author

@elasticmachine merge upstream

@davismcphee
Copy link
Contributor

/ci

@davismcphee
Copy link
Contributor

/ci

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

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
dataViewEditor 49.3KB 49.4KB +56.0B

History

cc @lsq645599166

@davismcphee
Copy link
Contributor

@elasticmachine run docs-build

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.

With the latest changes it LGTM! Thanks for working on this 👍

@davismcphee davismcphee merged commit 23848dd into elastic:main Oct 24, 2024
24 checks passed
@kibanamachine
Copy link
Contributor

Starting backport for target branches: 8.x

https://github.com/elastic/kibana/actions/runs/11491907929

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Oct 24, 2024
…lastic#195537)

## Summary

Closes elastic#194736

Show empty matching sources when no matched index pattern in create data
view panel

### Before

![image](https://github.com/user-attachments/assets/84bbc6b7-5ec7-4755-92fb-c2f52b38d7c1)

### After

![image](https://github.com/user-attachments/assets/af5ac7c2-4346-4f97-82aa-e4c54659d9a9)

---------

Co-authored-by: Matthias Wilhelm <[email protected]>
Co-authored-by: Elastic Machine <[email protected]>
Co-authored-by: Davis McPhee <[email protected]>
(cherry picked from commit 23848dd)
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

kibanamachine added a commit that referenced this pull request Oct 24, 2024
…ttern (#195537) (#197560)

# Backport

This will backport the following commits from `main` to `8.x`:
- [[DataView] show empty matching sources when no matched index pattern
(#195537)](#195537)

<!--- Backport version: 9.4.3 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Henry
Liu","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-10-24T03:09:51Z","message":"[DataView]
show empty matching sources when no matched index pattern
(#195537)\n\n## Summary\r\n\r\nCloses #194736\r\n\r\nShow empty matching
sources when no matched index pattern in create data\r\nview
panel\r\n\r\n###
Before\r\n\r\n![image](https://github.com/user-attachments/assets/84bbc6b7-5ec7-4755-92fb-c2f52b38d7c1)\r\n\r\n###
After\r\n\r\n\r\n![image](https://github.com/user-attachments/assets/af5ac7c2-4346-4f97-82aa-e4c54659d9a9)\r\n\r\n---------\r\n\r\nCo-authored-by:
Matthias Wilhelm <[email protected]>\r\nCo-authored-by:
Elastic Machine
<[email protected]>\r\nCo-authored-by: Davis
McPhee
<[email protected]>","sha":"23848ddea6185cadbf0fdd6fe59cd6b6b0c60ec0","branchLabelMapping":{"^v9.0.0$":"main","^v8.17.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["usability","release_note:skip","💝community","v9.0.0","Team:DataDiscovery","backport:prev-minor"],"title":"[DataView]
show empty matching sources when no matched index
pattern","number":195537,"url":"https://github.com/elastic/kibana/pull/195537","mergeCommit":{"message":"[DataView]
show empty matching sources when no matched index pattern
(#195537)\n\n## Summary\r\n\r\nCloses #194736\r\n\r\nShow empty matching
sources when no matched index pattern in create data\r\nview
panel\r\n\r\n###
Before\r\n\r\n![image](https://github.com/user-attachments/assets/84bbc6b7-5ec7-4755-92fb-c2f52b38d7c1)\r\n\r\n###
After\r\n\r\n\r\n![image](https://github.com/user-attachments/assets/af5ac7c2-4346-4f97-82aa-e4c54659d9a9)\r\n\r\n---------\r\n\r\nCo-authored-by:
Matthias Wilhelm <[email protected]>\r\nCo-authored-by:
Elastic Machine
<[email protected]>\r\nCo-authored-by: Davis
McPhee
<[email protected]>","sha":"23848ddea6185cadbf0fdd6fe59cd6b6b0c60ec0"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/195537","number":195537,"mergeCommit":{"message":"[DataView]
show empty matching sources when no matched index pattern
(#195537)\n\n## Summary\r\n\r\nCloses #194736\r\n\r\nShow empty matching
sources when no matched index pattern in create data\r\nview
panel\r\n\r\n###
Before\r\n\r\n![image](https://github.com/user-attachments/assets/84bbc6b7-5ec7-4755-92fb-c2f52b38d7c1)\r\n\r\n###
After\r\n\r\n\r\n![image](https://github.com/user-attachments/assets/af5ac7c2-4346-4f97-82aa-e4c54659d9a9)\r\n\r\n---------\r\n\r\nCo-authored-by:
Matthias Wilhelm <[email protected]>\r\nCo-authored-by:
Elastic Machine
<[email protected]>\r\nCo-authored-by: Davis
McPhee
<[email protected]>","sha":"23848ddea6185cadbf0fdd6fe59cd6b6b0c60ec0"}}]}]
BACKPORT-->

Co-authored-by: Henry Liu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) 💝community release_note:skip Skip the PR/issue when compiling release notes Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. usability v8.17.0 v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create data view - issues with index pattern and matching sources
6 participants