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

<Directory /> component refactored #80

Closed
wants to merge 9 commits into from
Closed

<Directory /> component refactored #80

wants to merge 9 commits into from

Conversation

isnifer
Copy link
Contributor

@isnifer isnifer commented Apr 4, 2017

Hey, @JiniHendrix, @markmarcelo, @bitadj, please take a look!

super();
}
export default class Directory extends Component {
handleListItemClick = proxyEvent => (

Choose a reason for hiding this comment

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

Arrow functions in classes are really hard to read. Maybe you should write them in a classical way like:

handleListItemClick(proxyEvent) {

    this.props.clickHandler(
        this.props.id,
        this.props.directory.path,
        this.props.directory.type,
        proxyEvent
    );
}

That change should also preserve a consistent code style because the handlers in the App component were not written as Arrow functions.

Also, wouldn't the methods handlePlusIconClick and handleListItemClick need to be bound to the this instance in the Component constructor?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're wrong. Arrow functions are easy to read. They help us to avoid using bind in constructor.
When we are writing arrow functions we do not need constructor and bind, because arrow functions already bound into an instance.

Just read - https://medium.com/@housecor/react-binding-patterns-5-approaches-for-handling-this-92c651b5af56

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What about code consistency - I've refactored all other files except App.jsx already. It will be next step

Choose a reason for hiding this comment

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

That are good points - Thanks, I didn't know that article! When the utilization of arrow functions makes the binding in classes obsolete, then it is a great design decision to use them. Overall I like your changes. You're awesome 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, Calvin! 👌

Copy link

Choose a reason for hiding this comment

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

@isnifer Probably if we can wrap the arrow function with the round parenthesis,
handleListItemClick = (proxyEvent) => {}
it would be more readable in terms of both classical and latest way of JavaScript coding pattern. Let me know your thoughts on this.

@isnifer
Copy link
Contributor Author

isnifer commented Apr 4, 2017

This branch was rebased over updated master

@bitadj bitadj force-pushed the master branch 2 times, most recently from 0cc2e45 to 6c2495c Compare April 5, 2018 12:50
@ryany1819
Copy link
Collaborator

As the main codebase shifted, this Pull Request created in 2017 was no longer applicable.

@ryany1819 ryany1819 closed this Jan 22, 2021
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.

None yet

8 participants