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
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions .babelrc
@@ -1,3 +1,3 @@
{
"presets": ["react", "es2015"]
}
"presets": ["react", "es2015", "stage-2"]
}
3 changes: 3 additions & 0 deletions .travis.yml
@@ -0,0 +1,3 @@
language: node_js
node_js:
- "6"
4 changes: 3 additions & 1 deletion package.json
Expand Up @@ -4,14 +4,15 @@
"description": "",
"main": "./main/main.js",
"scripts": {
"test": "mocha --compilers js:babel-register test/test.js",
"test": "mocha --compilers js:babel-register --reporter nyan test/test.js",
"start": "cross-env NODE_ENV=production electron .",
"dev-start": "cross-env NODE_ENV=development electron .",
"dev": "webpack-dev-server --inline --content-base ./renderer"
},
"author": "Mark Cruz, Jin Choi, Bita Djaghouri",
"license": "ISC",
"dependencies": {
"classnames": "^2.2.5",
"electron": "^1.6.1",
"electron-debug": "^1.1.0",
"electron-localshortcut": "^1.1.0",
Expand All @@ -29,6 +30,7 @@
"babel-loader": "^6.3.2",
"babel-preset-es2015": "^6.22.0",
"babel-preset-react": "^6.23.0",
"babel-preset-stage-2": "^6.22.0",
"babel-register": "^6.23.0",
"chai": "^3.5.0",
"chai-enzyme": "^0.6.1",
Expand Down
12 changes: 6 additions & 6 deletions renderer/components/App.jsx
Expand Up @@ -118,7 +118,7 @@ export default class App extends React.Component {
})
}
}
//handles click event from delete prompt
//handles click event from delete prompt
deletePromptHandler(answer) {
if (answer) {
ipcRenderer.send('delete', this.state.selectedItem.path);
Expand Down Expand Up @@ -280,20 +280,20 @@ export default class App extends React.Component {
}
}
//click handler for plus button on directories, 'opens' new file/dir menu by setting openMenuID state
openCreateMenu(id, itemPath, type, event) {
event.stopPropagation();
openCreateMenu(id, itemPath, proxyEvent) {
proxyEvent.stopPropagation();
this.setState({
openMenuId: id,
selectedItem: {
id: id,
path: itemPath,
type
type: null,
}
});
}
createForm(id, type, event) {
createForm(id, type, proxyEvent) {
document.body.onkeydown = () => { };
event.stopPropagation();
proxyEvent.stopPropagation();
this.setState({
createMenuInfo: {
id,
Expand Down
146 changes: 82 additions & 64 deletions renderer/components/Directory.jsx
@@ -1,76 +1,94 @@
import React from 'react';
import React, { Component } from 'react';
import cx from 'classnames';
import File from './File.jsx';
import CreateMenu from './CreateMenu.jsx';
import CreateForm from './CreateForm.jsx';
import RenameForm from './RenameForm.jsx';

export default class Directory extends React.Component {
constructor() {
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.

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

handlePlusIconClick = proxyEvent => (
this.props.openCreateMenu(
this.props.id,
this.props.directory.path,
proxyEvent
)
)

render() {
const arr = [];
let uniqueId;
for (var i = 0; i < this.props.directory.subdirectories.length; i++) {
arr.push(
<Directory
key={this.props.directory.subdirectories[i].id}
id={this.props.directory.subdirectories[i].id}
directory={this.props.directory.subdirectories[i]}
openFile={this.props.openFile}
clickHandler={this.props.clickHandler}
selectedItem={this.props.selectedItem}
openCreateMenu={this.props.openCreateMenu}
openMenuId={this.props.openMenuId}
createMenuInfo={this.props.createMenuInfo}
createForm={this.props.createForm}
createItem={this.props.createItem}
rename={this.props.rename}
renameHandler={this.props.renameHandler}
/>)
}
for (var i = 0; i < this.props.directory.files.length; i++) {
arr.push(
<File
key={this.props.directory.files[i].id}
id={this.props.directory.files[i].id}
file={this.props.directory.files[i]}
openFile={this.props.openFile}
clickHandler={this.props.clickHandler}
selectedItem={this.props.selectedItem}
rename={this.props.rename}
renameHandler={this.props.renameHandler}
/>)
}
let item = (
<div
className="list-item"
onClick={this.props.clickHandler.bind(null, this.props.id, this.props.directory.path, this.props.directory.type)}
>
const subdirectories = this.props.directory.subdirectories.map(directory => (
<Directory
key={directory.id}
id={directory.id}
directory={directory}
openFile={this.props.openFile}
clickHandler={this.props.clickHandler}
selectedItem={this.props.selectedItem}
openCreateMenu={this.props.openCreateMenu}
openMenuId={this.props.openMenuId}
createMenuInfo={this.props.createMenuInfo}
createForm={this.props.createForm}
createItem={this.props.createItem}
rename={this.props.rename}
renameHandler={this.props.renameHandler}
/>
));
const files = this.props.directory.files.map(file => (
<File
key={file.id}
id={file.id}
file={file}
openFile={this.props.openFile}
clickHandler={this.props.clickHandler}
selectedItem={this.props.selectedItem}
rename={this.props.rename}
renameHandler={this.props.renameHandler}
/>
));
const directory = subdirectories.concat(files);

const item = (
<div className="list-item" onClick={this.handleListItemClick}>
<span className="icon icon-file-directory">
{this.props.directory.name}
</span>
<span className="plus-icon" onClick={this.props.openCreateMenu.bind(null, this.props.id, this.props.directory.path)}>+</span>
{this.props.openMenuId === this.props.id ? <CreateMenu createForm={this.props.createForm} id={this.props.id} /> : <span />}
{this.props.createMenuInfo.id === this.props.id ? <CreateForm createItem={this.props.createItem} /> : <span />}
</div>)
if (this.props.directory.opened) {
return (
<li className={this.props.selectedItem.id === this.props.id ? 'list-nested-item selected' : 'list-nested-item'}>
{this.props.rename && this.props.selectedItem.id === this.props.id ? <RenameForm renameHandler={this.props.renameHandler} /> : item}
<span className="plus-icon" onClick={this.handlePlusIconClick}>+</span>
{this.props.openMenuId === this.props.id ?
<CreateMenu createForm={this.props.createForm} id={this.props.id} /> :
<span />
}
{this.props.createMenuInfo.id === this.props.id ?
<CreateForm createItem={this.props.createItem} /> :
<span />
}
</div>
);

const listItemClassName = cx('list-nested-item', {
collapsed: !this.props.directory.opened,
selected: this.props.selectedItem.id === this.props.id
});

return (
<li className={listItemClassName}>
{this.props.rename && this.props.selectedItem.id === this.props.id ?
<RenameForm renameHandler={this.props.renameHandler} /> :
item
}
{this.props.directory.opened &&
<ul className="list-tree">
{arr}
{directory}
</ul>
</li>
)
} else {
return (
<li
className={this.props.selectedItem.id === this.props.id ? 'list-nested-item collapsed selected' : 'list-nested-item collapsed'}
>
{this.props.rename && this.props.selectedItem.id === this.props.id ? <RenameForm renameHandler={this.props.renameHandler} /> : item}
</li>
)
}
}
</li>
)
}
}
}