[GH-394] Migration to TypeScript: Sidebar_button migrated to Typescript#438
[GH-394] Migration to TypeScript: Sidebar_button migrated to Typescript#438mickmister merged 11 commits intomattermost:masterfrom Willy-Wakam:master
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #438 +/- ##
=======================================
Coverage 33.40% 33.40%
=======================================
Files 22 22
Lines 3979 3979
=======================================
Hits 1329 1329
Misses 2519 2519
Partials 131 131 ☔ View full report in Codecov by Sentry. |
Willy-Wakam
changed the title
mickmister
left a comment
mickmister
left a comment
There was a problem hiding this comment.
Thanks for this contribution @Willy-Wakam! This is looking really good. I have just a few suggestions. Also, we'll want to convert src/components/sidebar_buttons/index.js to typescript if possible.
To fix some linting rules, I suggest changing these lines in .eslintrc.json:
"no-magic-numbers": "off",
"no-use-before-define": "off",
Note that no-use-before-define is superseded by @typescript-eslint/no-use-before-define, so those checks will still be made.
Can you please run this command locally to ensure the linting is correct? The --fix flag makes it automatically fix certain things as well.
./node_modules/.bin/eslint src/components/sidebar_buttons/sidebar_buttons.tsx --fixI tried to run the below command locally but it doesn't seem to work. It spits out a bunch of errors from node_modules/@types only. Using your editor's error reporting in that file should be enough for tsc errors.
npx tsc src/components/sidebar_buttons/sidebar_buttons.tsx --noEmit --skipLibCheck| import React from 'react'; | ||
| import {Tooltip, OverlayTrigger} from 'react-bootstrap'; | ||
| import React, { PureComponent, ReactElement } from 'react'; | ||
| import { Tooltip, OverlayTrigger } from 'react-bootstrap'; |
There was a problem hiding this comment.
Our convention is generally to not have spaces in the imports. Our linter should catch this, though on a closer inspection, the npm run lint command is configured to only check js and jsx files
|
@ayusht2810 Heads up that the linter seems to be configured incorrectly in |
ayusht2810
left a comment
ayusht2810
left a comment
There was a problem hiding this comment.
LGTM! Apart from @mickmister comments.
|
|
||
| import { RHSStates, connectUsingBrowserMessage } from 'src/constants'; | ||
| import { isDesktopApp } from 'src/utils/user_agent'; | ||
| // import PropTypes from 'prop-types'; |
There was a problem hiding this comment.
We can remove this comment if it's of no use
There was a problem hiding this comment.
why are we having changes in this file even though we didn't make any changes in the package.json file?
There was a problem hiding this comment.
I got some errors in the previous commit, so I deleted and pulled the forked project again. I guess by installing node modules, the file was edited, although it was not really changed
There was a problem hiding this comment.
The package-lock.json says it's lockfileVersion: 3 now. @Willy-Wakam Can you please either revert those changes, or try re-running npm install in the webapp folder after setting your node version to the version defined here?
mattermost-plugin-gitlab/.nvmrc
Line 1 in eab8193
You can set your node version using tools like https://github.com/nvm-sh/nvm or https://github.com/Schniz/fnm
|
If there are no more relevant changes, could you @ayusht2810 please merge and close the PR? |
mickmister
left a comment
mickmister
left a comment
There was a problem hiding this comment.
LGTM, thanks @Willy-Wakam!
|
Actually blocking on #438 (comment), since this PR currently changes the lockfile version to 3, when it should be 2 |
ayusht2810
left a comment
ayusht2810
left a comment
There was a problem hiding this comment.
LGTM! Apart from lock file changes.
|
Thanks for the contribution @Willy-Wakam! |
4 participants
Summary
The file sidebar_button.jsx was migrated into his appropriated Typescript version
All properties and classes were well typed
Ticket Link
Fixes #430