-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Discover] Refactor discover.js controller topnav code #79062
[Discover] Refactor discover.js controller topnav code #79062
Conversation
…-01-discover-refactor-topnav-code
@elasticmachine merge upstream |
…github.com:kertal/kibana into kertal-pr-2020-10-01-discover-refactor-topnav-code
…-01-discover-refactor-topnav-code
import { updateSearchSource } from '../helpers/update_search_source'; | ||
import { calcFieldCounts } from '../helpers/calc_field_counts'; | ||
|
||
const services = getServices(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that the IDE did some of this reorderings, import before variable declaration, sort import alphabetical, kept it because they make sense
stateValFound: !!index && id === index, | ||
}); | ||
}), | ||
ip: loadIndexPattern(index, data.indexPatterns, config), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that the refactoring here is a first step, separate loading and resolving shouldn't be necessary, it improving it step by step
removeField<K extends keyof SearchSourceFields>(field: K) { | ||
delete this.fields[field]; | ||
return this; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did I add this? To prevent TypeScript confusion, because when I used 'setFields(key, null)' to remove it, I got depending what I was trying to remove TypeScript errors
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nice if we could add a quick unit test for the new method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Absolutely! I've added a jest test.
…-01-discover-refactor-topnav-code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just looked at app services code -- The searchSource.removeField
update makes sense to me 👍
@majagrubic from my side it should be fine, let me know if there's something else I should do |
@elasticmachine merge upstream |
@elasticmachine merge upstream |
…github.com:kertal/kibana into kertal-pr-2020-10-01-discover-refactor-topnav-code
@elasticmachine merge upstream |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't tested it since the last revision, but code changes look good 👍
…github.com:kertal/kibana into kertal-pr-2020-10-01-discover-refactor-topnav-code
💚 Build SucceededMetrics [docs]Module Count
Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: |
* Move discover.js functions to helper functions in separate files * Convert to TypeScript * Add unit tests * Add removeField function to SearchSource
Summary
This PR extracts logic & functionality of discover.js (containing the angular controller) related to the topnav/toolbar, field counting, search source updating to separate files and functions, converting to TypeScript.
Testing
There shall be no regressions!
Checklist