-
Notifications
You must be signed in to change notification settings - Fork 944
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
[CCI] Fix type errors in Saved Object Management #3987
[CCI] Fix type errors in Saved Object Management #3987
Conversation
Signed-off-by: Alexei Karikov <[email protected]>
Signed-off-by: Alexei Karikov <[email protected]>
Signed-off-by: Alexei Karikov <[email protected]>
Signed-off-by: Alexei Karikov <[email protected]>
I've fixed most of the errors, could you check? Could you suggest how best to fix the errors in
|
@Nicksqain Some of the unit test snapshots need to be updated. You can do that via |
Signed-off-by: Alexei Karikov <[email protected]>
Signed-off-by: Alexei Karikov <[email protected]>
I have updated the snapshot, which should solve the problem |
Signed-off-by: Alexei Karikov <[email protected]>
Signed-off-by: Alexei Karikov <[email protected]>
onTableChange: (table: any) => void; | ||
isSearching: boolean; | ||
onShowRelationships: (object: SavedObjectWithMetadata) => void; | ||
canGoInApp: (obj: SavedObjectWithMetadata) => boolean; | ||
dateFormat: string; | ||
dateFormat?: string; |
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'm not sure about the optional dateFormat
props because it's not used in the defaultProps
src\plugins\saved_objects_management\public\management_section\objects_table\components\table.test.tsx test
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 think making it optional is probably fine - moment will accept undefined
as an input on L251. Alternatively you could just update the test defaultProps
to include it, like in
Line 164 in 07e9773
dateFormat: 'YYYY-MM-DD', |
@@ -161,6 +161,7 @@ describe('SavedObjectsTable', () => { | |||
goInspectObject: () => {}, | |||
canGoInApp: () => true, | |||
search, | |||
dateFormat: 'YYYY-MM-DD', |
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.
Can you tell me what value the dateFormat should take? In
src\plugins\saved_objects_management\public\management_section\saved_objects_table_page.tsx
the value is
const dateFormat = coreStart.uiSettings.get<string>('dateFormat');
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.
That's a good question. It's a little hard to tell, but to be consistent with the ui_settings test, I'd use Browser
as the default test string:
const { defaults = { dateFormat: { value: 'Browser' } }, initialSettings = {} } = options; |
Codecov Report
@@ Coverage Diff @@
## main #3987 +/- ##
==========================================
- Coverage 66.36% 66.30% -0.06%
==========================================
Files 3229 3229
Lines 62145 62147 +2
Branches 9622 9623 +1
==========================================
- Hits 41240 41205 -35
- Misses 18584 18615 +31
- Partials 2321 2327 +6
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Signed-off-by: Josh Romero <[email protected]>
(I resolved CHANGELOG conflicts) |
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.
Mostly looking good, a couple minor questions.
} catch (respError: any) { | ||
const respBody = get(respError, 'data', {}) as any; |
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.
nit - in general, prefer unknown
to any
.
private readonly alias; | ||
private alias: string = ''; |
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.
This changes the behavior a bit. Is there any error cases or edge case handling that needs to be changed to handle ''
vs undefined
as the default value?
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.
PR lgtm. There's a snapshot test failed from CI. Can you update snapshots?
PR has been stale for 10 months. Closing. Feel free to re-open if you plan to work on it again. @Nicksqain |
Description
Issues Resolved
Check List
yarn test:jest
yarn test:jest_integration
yarn test:ftr