-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Conversation
Here is a PR for a basic create react app. It has basic create react app, husky, commitzen, and and commitlint. This should address all tasks that were in the issue. I have marked it as WIP since I have the following questions.
|
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.
Update package.json to include @hospitalrun-org
namespace and pre-commit hooks
|
@PhearZero I can start working on getting a templates repository put together. I think it's probably outside the scope of this PR |
|
module.exports = { | ||
extends: ["gitmoji"], | ||
parserPreset: { | ||
parserOpts: { |
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.
@tehkapa is this compatible with the commit style you proposed?
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.
theoretically, yes. however, I also extends this ['@commitlint/config-conventional']
@PhearZero did you use both solutions in your tests?
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.
I really like conventional. In addition to it we need also to enforce referencing the issue the commits are addressing.
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.
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.
Is there anything that I need to do here?
This looks like it matches the config in the hospitalrun/hospitalrun repository. Also matches the config propoed in HospitalRun/hospitalrun-server#154
If we are changing, I would assume we would update this config to match hospitalrun/components
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.
Please remove all react logos: we will put ours instead.
"eject": "react-scripts eject" | ||
}, | ||
"eslintConfig": { | ||
"extends": "react-app" |
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.
Is this eslintConfig compatible with TS? I can't see the language plugins.
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.
It's what the CRA outputs with the --typescript flag. It's somewhere in react-scripts that handles all of the heavy lifting. Not very familiar with the cli yet. We could eject the app but we lose support from upstream
@jackcmeyer we will use always the same tsconfig. This is our FE config https://github.com/HospitalRun/components/blob/master/tsconfig.json |
Just a note react-scripts ignores/overrides some tsconfig options so we may have to eject if we want full control or find a workaround. Ran into this issue trying to resolve module aliases |
I've updated the |
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.
proposed change to the dockerfile
@irvelervel what do you think about what is proposing @jackcmeyer? |
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.
Good to go!
fixes #1619
Got some code for us? Awesome 🎊!
Please include a description of your change & check your PR against this list, thanks!
More info can be found by clicking the "guidelines for contributing" link above.