Upgraded to [email protected], [email protected], [email protected], [email protected] #886
Conversation
Needed to remove a line in the PlotBase definition due to the upgrade of @types/react Have to change protected to public for componentWillUnmount() function
Merged the upgrades from upgrade_webpack branch with typescript 3.4.1
yarn built successfully, and passes all the tests. However the webpack-dev-server renders a blank screen which still need to be solved.
Co-authored-by: Dominik Moritz <[email protected]>
Co-authored-by: Dominik Moritz <[email protected]>
And resolved the style issue.
|
I will work on addressing the comments |
However, there still are errors when running jest
|
This pull request introduces 3 alerts when merging 8822adf into 882809f - view on LGTM.com new alerts:
|
|
This pull request introduces 2 alerts when merging d86b3e7 into 882809f - view on LGTM.com new alerts:
|
|
This pull request introduces 2 alerts when merging 40bec63 into 882809f - view on LGTM.com new alerts:
|
|
Please fix the tests |
|
This pull request introduces 2 alerts when merging 6d04f84 into 2bd9d86 - view on LGTM.com new alerts:
|
|
Let me know when tests pass. |
There was a problem hiding this comment.
Very nice!
Please remove yarn-error.log.
Did you check that the app works as expected? If so, I will make a final pass and merge after you address my comments.
config/webpack.config.lib.js
Outdated
| @@ -6,10 +6,10 @@ const CaseSensitivePathsPlugin = require('case-sensitive-paths-webpack-plugin'); | |||
| const TerserPlugin = require("terser-webpack-plugin"); | |||
| var WebpackNotifierPlugin = require('webpack-notifier'); | |||
|
|
|||
| const getClientEnvironment = require('./env'); | |||
| // const getClientEnvironment = require('./env'); | |||
There was a problem hiding this comment.
please remove commented out code
config/webpack.config.lib.js
Outdated
| var publicUrl = ''; | ||
| const env = getClientEnvironment(publicUrl); | ||
| // var publicUrl = ''; | ||
| // const env = getClientEnvironment(publicUrl); |
|
Thanks! I just addressed the comments and deleted the unavailable datasets, and I have tested that the app works as expected. Would you like me to add more datasets from vega-datasets? |
|
@siyaoL1 is working on support CSV and then we can merge this. |
Now the default dataset supports csv files from vega_datasets, such as birdstricks.csv.
|
This pull request introduces 1 alert when merging 0bd2706 into 2bd9d86 - view on LGTM.com new alerts:
|
List of upgraded dependencies: