-
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
[FieldFormatters] Lazy load field formatter editors #106439
Conversation
|
||
export interface FormatEditorProps { | ||
fieldType: string; | ||
fieldFormat: FieldFormat; | ||
fieldFormatId: string; | ||
fieldFormatParams: { [key: string]: unknown }; | ||
fieldFormatEditors: any; | ||
onChange: (change: { fieldType: string; [key: string]: any }) => void; |
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.
After improving types, typescript highlighted type mismatch here. Looks like there is no fieldType: string;
emitted from the editors so I removed it from here to fix types
}; | ||
|
||
describe('FieldFormatEditor', () => { | ||
it('should render normally', async () => { | ||
const component = shallow( | ||
<FieldFormatEditor | ||
fieldType="number" | ||
fieldFormat={{} as DefaultFormatEditor} |
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.
Apparently, this type was incorrect. Fixed casting in the test and in the file itself.
|
||
export class FieldFormatEditors { | ||
private editors: Array<typeof DefaultFormatEditor> = []; | ||
private editors: FieldFormatEditorFactory[] = []; |
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.
Now instead of registering a component, we require to register a factory that creates the component. This allows to load the component async
@elasticmachine merge upstream |
Pinging @elastic/kibana-app-services (Team:AppServices) |
@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.
changes look good and work well
@elasticmachine merge upstream |
💛 Build succeeded, but was flaky
Test FailuresKibana Pipeline / general / Chrome UI Functional Tests.test/functional/apps/discover/_indexpattern_without_timefield·ts.discover app indexpattern without timefield should switch between with and without timefield using the browser back buttonStandard Out
Stack Trace
Metrics [docs]Module Count
Public APIs missing comments
Async chunks
Public APIs missing exports
Page load bundle
Unknown metric groupsAPI count
async chunk count
History
To update your PR or re-run it, just comment with: |
💚 Backport successful
This backport PR will be merged automatically after passing CI. |
Co-authored-by: Anton Dosov <[email protected]>
Summary
close #100366
Page load bundle
Size improvement just 36Kb, but I still think it is worth it for such a simple change + it will add up as/if we get more field formatters and more complicated editors
How to test
I found the usages editors in the following places. Hope I am not missing anything. There is also a bit of code duplication between those:
src/plugins/index_pattern_management/public/components/field_editor/components/field_format_editor/field_format_editor.tsx
:src/plugins/index_pattern_field_editor/public/components/field_format_editor/format_editor.tsx
: