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

Update tab counters on filter change #34246

Merged
merged 2 commits into from
Apr 10, 2019

Conversation

flash1293
Copy link
Contributor

Summary

Fixes #19524 by updating the tab counters if the filter is changed.

@timroes The fix is in the angular code, so a complete EUI-ification of the edit index pattern section would fix this problem. If we get this in before 7.1 feature freeze, we don't need this PR.

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

- [ ] This was checked for cross-browser compatibility, including a check against IE11
- [ ] 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
- [ ] This was checked for keyboard-only and screenreader accessibility

For maintainers

@flash1293
Copy link
Contributor Author

flash1293 commented Apr 1, 2019

Related: I noticed a small error in kbn-ui - the selected tab doesn't stand out anymore (due to a change during dark mode integration). Is this worth fixing?
Old:
Screenshot 2019-04-01 at 10 06 27

New:
Screenshot 2019-04-01 at 09 57 51
Screenshot 2019-04-01 at 10 04 38

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@timroes
Copy link
Contributor

timroes commented Apr 1, 2019

@flash1293 I was wondering (maybe you know the code now to estimate this), if it's worth when filtering down, actually not showing the plain number of now found fields, but instead something like Fields (9/85) or Fields (9 of 85)?

@flash1293
Copy link
Contributor Author

@timroes This is easy to add to the current state. I also forgot to filter the counts for the source filters, I will add this to the PR.

@flash1293
Copy link
Contributor Author

This doesn't respect the language filter in the "Scripted fields" tab - does this even work in the newer versions? Can this dropdown be removed completely?

The control flow is a little awkward, because the individual tables do their own filtering (we basically filter twice, once in angular for the tabs, once in react for the actual table). But if we fix this now we might as well EUI-ify the whole thing which is a little more effort.

@flash1293
Copy link
Contributor Author

I looked into the code and in
src/legacy/ui/public/scripting_languages/index.js and src/legacy/core_plugins/kibana/server/routes/api/scripts/register_languages.js it is hardcoded that there will only be painless. Is there a reason why the code around this is still kept in place?

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@timroes
Copy link
Contributor

timroes commented Apr 2, 2019

You can still create a scripted field with the language expression for numeric fields, having some (I think) Lucene calculation language.

@flash1293 flash1293 added Feature:Kibana Management Feature label for Data Views, Advanced Setting, Saved Object management pages Team:Visualizations Visualization editors, elastic-charts and infrastructure labels Apr 3, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app

@flash1293
Copy link
Contributor Author

@timroes Not since late 2017 (at least according to #14310 ) - the code also looks like this is the case. Maybe I overlook something

Copy link
Contributor

@timroes timroes left a comment

Choose a reason for hiding this comment

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

Tested on Chrome Linux. Seems to work fine. Code LGTM.
I would leave the code for multiple languages for now in there. Maybe if we refactor that part we can discuss throwing it out, but I see not current need for that (and I anyway wouldn't do it in this PR :D)

@flash1293 flash1293 merged commit b7b584c into elastic:master Apr 10, 2019
@flash1293 flash1293 deleted the 19524/tab-counter-updates branch April 10, 2019 13:41
flash1293 added a commit to flash1293/kibana that referenced this pull request Apr 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Kibana Management Feature label for Data Views, Advanced Setting, Saved Object management pages release_note:fix Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.2.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tab counter not updating when filtering search results in index pattern
3 participants