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
refactor: typing for explore Control and messageToasts #11416
Conversation
Codecov Report
@@ Coverage Diff @@
## master #11416 +/- ##
==========================================
- Coverage 66.49% 62.31% -4.19%
==========================================
Files 860 860
Lines 40869 40871 +2
Branches 3686 3694 +8
==========================================
- Hits 27176 25467 -1709
- Misses 13592 15224 +1632
- Partials 101 180 +79
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
owners: PropTypes.arrayOf(PropTypes.string), | ||
owners: PropTypes.arrayOf( | ||
PropTypes.oneOfType([PropTypes.string, PropTypes.number]), | ||
), |
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.
By catch: suppress a propTypes warning. Chart's ProprtiesModal loads owners
via usernames, but when updating, it updates to numeric IDs.
export const UPDATE_EXPLORE_ENDPOINTS = 'UPDATE_EXPLORE_ENDPOINTS'; | ||
export function updateExploreEndpoints(jsonUrl, csvUrl, standaloneUrl) { | ||
return { type: UPDATE_EXPLORE_ENDPOINTS, jsonUrl, csvUrl, standaloneUrl }; | ||
} |
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 action is added in a 4 yo PR but doesn't seem to have been used anywhere (even in the original PR).
sliceUpdated, | ||
}; | ||
|
||
export type ExploreActions = typeof exploreActions; |
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.
Explore and enumerate all action functions so it can be used in external typing.
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.
LGTM
}; | ||
|
||
const thunk = handleComponentDrop(dropResult); | ||
thunk(dispatch, getState); | ||
expect(dispatch.getCall(0).args[0].type).toEqual( | ||
addWarningToast('').type, | ||
); |
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.
Bycatch: this was an invalid test. The invalid input above didn't actually test the case it was supposed to test ("drop overflows the destination").
Previously this test passes because both dispatch.getCall(0).args[0].type
and addWarningToast('').type
were undefined
, when they both should be ADD_TOAST
. This error was surfaced because I removed the dispatch
wrapper for toast actions.
SUMMARY
Needed to do some refactor with chart controls. Finally adding typing to this very important component so to make future changes safer.
Updated typing for
messageToasts
actions, too, because it's a dependency ofexploreActions
.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A
TEST PLAN
CI and manually testing to make sure controls still work.
ADDITIONAL INFORMATION