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

Create new starter files for different React components #41

Conversation

jatin11gidwani
Copy link
Collaborator

Created new starter files for different react components including FAQs,
GameDisplay, HowToPlay, NavigationMenu, QuoteAndOptions and Score.

Created new starter files for different react components including FAQs,
GameDisplay, HowToPlay, NavigationMenu, QuoteAndOptions and Score.
@@ -0,0 +1,7 @@
{
Copy link
Contributor

@deveshjadon98 deveshjadon98 Apr 23, 2018

Choose a reason for hiding this comment

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

We don't need to have package.json along with every component, it is used to store app wide dependencies. All dependencies for these components should be defined in this package.json
Please remove package.json file from all the components.

Copy link
Collaborator

@skyshader skyshader Apr 24, 2018

Choose a reason for hiding this comment

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

This is required in order to access the component directly from other files. So if you do import FAQs from '../FAQs'; you'd be able to do that. But if you don't have a package.json file in component directory you'd have to do import FAQs from '../FAQs/FAQs'; which to me sounds confusing. So, we are not defining any dependencies here, we are defining the entry point for this component and a version mostly.

Other option is to use a index.js file in the folders for writing components, but then you have confusion on your editor tabs showing index.js for every file open which confuses poeple.

Another option is to have a index.js file which exports the Component.js file. So that you can achieve the same thing as above.

Let me know what you think @deveshjadon98, @twishasaraiya, @anantoghosh and we can decide on sticking with any one of the above approaches.

Copy link
Contributor

Choose a reason for hiding this comment

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

creating a pipeline by exporting components from a index.js file is an industry standard and helps in destructuring import( import only what you need ) as well. The better approach would be to have a single index.js file in components directory and all the components should be exported from that one particular file.
Using package.json is more confusing to me or any other developer who'll start contributing on this project.

  • Components
    • FAQs
    • GameDisplay
    • index.js

Copy link
Collaborator

Choose a reason for hiding this comment

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

Well, In my opinion that would be over-doing things and has some disadvantages associated with it.

  1. Every contributor working on different components will be updating same index.js file causing many conflicts.
  2. The component is no longer independent as a package.

We can think about moving to index.js approach but keep it in every component folder exporting each component individually in their own directory.

@deveshjadon98 let me know your thoughts.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's proceed with separate index.js file with every component.

Copy link
Contributor

Choose a reason for hiding this comment

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

@skyshader the index approach is better for refactoring.
With package.json approach, in case we wanted to change a component name, then we'll have to:

  • Change the folder name
  • Change the js file name to match
  • Edit the package.json to match

Given that we'll have to do it manually every time, this would be easy to mess up.
With index, only the folder name has to be changed.
Most IDEs do show the module name along the index file.

On a different note, the package.json module resolution approach is specific to node. Webpack also uses package.json for compatibility reasons. But this is not the es6 standard import. This will not work anywhere else - with native es6 import, typescript or any other bundler.
Many of the contributers here are beginners and I don't think it would be right to show them the component based package.json is the way to do things.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, so now we have decided to go with the 3rd approach recommended @anantoghosh
Instead of package.json we would have index.js which will export our AnyComponent.js from that directory.

@deveshjadon98 will you be able to submit a PR for this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Already working on it ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

I have submitted the PR related to this discussion, please check.
#46


class FAQs extends Component {

render () {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@jatin11gidwani Components are just presentation.They will simply return the formatted data(which will be passed as props) which will be rendered from the container.No need for render() here just return it.
return (<div>FAQs</div>)
Also we need to define PropTypes
Please refer SampleComponent.js

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, do end the file with a new line "Always" :)

@twishasaraiya
Copy link
Collaborator

@jatin11gidwani @skyshader @deveshjadon98 @anantoghosh Lets first discuss it here.Please let me you know what you guys think. Let's get things finalized first. Till then I am closing this PR.

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

Successfully merging this pull request may close these issues.

5 participants