package: upgrade to react-scripts 5.0.0
#213
Conversation
- upgrade semantic-ui deps to latest - install `@semantic-ui-react/css-patch` to fix `semantic-ui-css` issue (Semantic-Org/Semantic-UI-React#4287 (comment)) closes reanahub#204
- `semantic-ui-less` is used instead for styling due to theming - more info: https://react.semantic-ui.com/theming/
There was a problem hiding this comment.
Looks good and works fine for me.
I noticed that now we have both yarn.lock and package-lock.json which could be confusing. Since we switched completely to yarn, perhaps we can get rid of the package-lock.json? Otherwise it would be good to update it as well..
By the way, while installing I got this warning from yarn:
Warning package-lock.json found. Your project contains lock files generated by tools other than Yarn. It is advised not to mix package managers in order to avoid resolution inconsistencies caused by unsynchronized lock files. To clear this warning, remove package-lock.json.
run-tests.sh
Outdated
| @@ -20,8 +20,12 @@ check_sphinx () { | |||
| sphinx-build -qnNW docs docs/_build/html | |||
| } | |||
|
|
|||
| check_lint () { | |||
| cd reana-ui && yarn lint && cd .. | |||
There was a problem hiding this comment.
Minor: another option would be to use parentheses in commands. In this way we don't have to go back (cd ..). Command would look like this: (cd reana-ui && yarn lint). Same could be applied for check_prettier and check_js_tests. WDYT?
There was a problem hiding this comment.
Make sense, didn't know the subshell functionality 👍
| @@ -55,6 +59,7 @@ do | |||
| case $arg in | |||
| --check-shellscript) check_script;; | |||
| --check-sphinx) check_sphinx;; | |||
| --check-lint) check_lint;; | |||
There was a problem hiding this comment.
We should also add check_lint to check_all() and probably add a new GA CI job as well?
Minor: year was not updated in this file
There was a problem hiding this comment.
What about GA CI job for linting? Don't we need it?
| "fmt": "prettier --write .", | ||
| "ci": "run-p lint prettier", | ||
| "eject": "craco eject", | ||
| "postinstall": "semantic-ui-css-patch" |
There was a problem hiding this comment.
Lots of new commands were added, it's not completely clear for me when one has to use it? For example postinstall command is triggered automatically or one has to do it at some point? Maybe it would be worth to have a short description in the readme?
There was a problem hiding this comment.
There was a problem hiding this comment.
Would something like this make sense?
Yarn scripts
============
- `start`: start a development server
- `build`: build a production-ready bundle in the `build` folder
- `test`: run unit tests
- `lint`: run linter
- `prettier`: check code formatting with prettier
- `fmt`: fix formatting problems with prettier
- `ci`: run both linter and format checkers, useful before committing changes
Most of them are very repetitive but it might help, not sure, let me know.
There was a problem hiding this comment.
looks nice and clear in my opinion! thanks
Where's the |
Indeed it seems it was generated by me locally and because of |
closes #204