Migration to TypeScript: Sidebar_button migrated to Typescript #450
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #450 +/- ##
=======================================
Coverage 33.53% 33.53%
=======================================
Files 22 22
Lines 3996 3996
=======================================
Hits 1340 1340
Misses 2523 2523
Partials 133 133 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Thanks for this contribution @Willy-Wakam 👍
The changes look good. I have just one request type the related index.ts (which is straightforward in this case), and a suggestion to convert an existing React class component into a function component. Please let me know what you think
| @@ -1,6 +1,6 @@ | |||
| import {connect} from 'react-redux'; | |||
|
|
|||
| import SidebarHeader from './sidebar_header.jsx'; | |||
| import SidebarHeader from './sidebar_header'; | |||
|
|
|||
| function mapStateToProps(state) { | |||
| const members = state.entities.teams.myMembers || {}; | |||
There was a problem hiding this comment.
@Willy-Wakam What are your thoughts on typing this file to index.ts as well? We can use this type for the state parameter:
import {GlobalState} from 'mattermost-redux/types/store';
function mapStateToProps(state: GlobalState) {| interface SidebarHeaderProps{ | ||
| show: boolean, | ||
| theme: Theme | ||
| } |
There was a problem hiding this comment.
Typically we would have the type like this to differentiate types from real data, though it's not configured in eslint for some reason
| interface SidebarHeaderProps{ | |
| show: boolean, | |
| theme: Theme | |
| } | |
| interface SidebarHeaderProps { | |
| show: boolean; | |
| theme: Theme; | |
| } |
| export default class SidebarHeader extends PureComponent<SidebarHeaderProps> { | ||
| render(): ReactElement | null { | ||
| if (!this.props.show) { | ||
| return null; |
There was a problem hiding this comment.
Can we convert this into a function component in this PR?
| export default class SidebarHeader extends PureComponent<SidebarHeaderProps> { | |
| render(): ReactElement | null { | |
| if (!this.props.show) { | |
| return null; | |
| export default function SidebarHeader(props: SidebarHeaderProps): React.ReactNode { | |
| if (!props.show) { | |
| return null; |
There was a problem hiding this comment.
@Willy-Wakam This actually isn't necessary for this PR
|
@Willy-Wakam Are you planning to continue work on this PR anytime soon? |
Summary
The file sidebar_header.jsx was migrated into his appropriated Typescript version
All properties and classes were well typed
Ticket Link
Fixes #431