Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

replace create-react-app with create-react-library #78

Merged
merged 7 commits into from
May 10, 2019
Merged

Conversation

orlando
Copy link
Contributor

@orlando orlando commented May 10, 2019

What: replace create-react-app with create-react-library

Why: work related to debtcollective/disputes#14

How: created a project with create-react-library and moved the files over the header repo once everything was working

Notes

create-react-library

create-react-library is better oriented to create React component libraries than create-react-app. It is based on Rollup which now seems to be the default for creating components (vs webpack which is more oriented to applications).

The build process now works correctly, managing peerDependencies correctly, you can see it working in the example folder.

pretty-quick

pretty-quick doesn't have integration with ESLint (prettier/pretty-quick#22). The problem was that pretty-quick had a different output than the eslint command, and it wasn't using the eslint configuration, which is not what we want.
Prettier has prettier-eslint, we are using through prettier-eslint-cli instead of pretty-quick. This runs prettier and ESLint in one pass.

Let's use this setup in other projects and tweak it as we need it.

gh-pages

We are deploying the example to Github pages https://debtcollective.github.io/header/. This should be done by the CI every time we merge to master.

@orlando orlando requested a review from duranmla May 10, 2019 00:15
@codecov
Copy link

codecov bot commented May 10, 2019

Codecov Report

Merging #78 into master will increase coverage by 2.28%.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #78      +/-   ##
=======================================
+ Coverage   93.71%   96%   +2.28%     
=======================================
  Files          24    23       -1     
  Lines         207   200       -7     
  Branches       15    14       -1     
=======================================
- Hits          194   192       -2     
+ Misses         12     8       -4     
+ Partials        1     0       -1
Impacted Files Coverage Δ
src/profile/InboxDropdown.js 100% <ø> (ø)
src/locales/index.js 100% <ø> (ø)
src/__fixtures__/notifications.js 100% <ø> (ø)
src/appbar/AppBar.js 100% <ø> (ø)
src/profile/ProfileItems.js 100% <ø> (ø)
src/profile/UserAvatarDropdown.js 100% <ø> (ø)
src/menu/Menu.js 100% <ø> (ø)
src/session/Session.js 83.33% <ø> (ø)
src/notifications/NotificationService.js 87.5% <ø> (ø)
src/notifications/normaliser.js 100% <ø> (ø)
... and 13 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 17ddc55...5279dd7. Read the comment docs.

src/.eslintrc Outdated
@@ -0,0 +1,5 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we have two files of .eslintrc?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question, this was there when I copied the files over. I’ll remove it

}

return discourseEndpoint;
return process.env.DISCOURSE_ENDPOINT || "http://localhost:3000";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we built passing an environment variable I guess right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, we need to check how to make this work. If there's more work to do when integrating with the disputes app I'll create a ticket for it

@duranmla
Copy link
Contributor

This looks great, couple questions but nothing blocker.
I do not know if it is worthy to add some pending tasks to revise later related to:

  • Check the usage of the environment variable to see if worthy to use config based approach
  • docz currently we do not have a way to make sure the docz are not broken when we push to master (a sort of checking if it built correctly or publishing - I do not know if worthy neither -)

@duranmla
Copy link
Contributor

Nice work 👌

"eslint-plugin-flowtype": "~3.8.1",
"eslint-plugin-import": "^2.13.0",
"eslint-plugin-node": "^7.0.1",
"eslint-plugin-promise": "^4.0.0",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW, having these plugins from eslint but not in the configuration sounds like we are not actually using it.
Are you positive about their usage? I just see in the config flow and react

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let’s talk about this, by default this usess standard js and I would like to use something like that

@orlando orlando merged commit cc9826f into master May 10, 2019
@orlando orlando deleted the od/build branch May 10, 2019 17:06
@orlando
Copy link
Contributor Author

orlando commented May 10, 2019

🎉 This PR is included in version 1.6.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants