because the task #1

trypolski · 2018-06-12T14:43:17Z

it was interesting

christianbirg · 2018-06-27T09:07:21Z

src/app/store.js


 export const store = createStore(reducer, middleware)

+// Saving the store changing to the Local Storage
+store.subscribe(() => {


nice solution.

My idea where to hydrate/ryhydrate the store with a solution outthere. But i such a simple usecase its a very nice solution with no overhead.

christianbirg · 2018-06-27T09:13:18Z

src/app/components/Columns/BookmarkAddColumn.js

+      onChange={onChange}
+      onKeyPress={onKeyPress} />
+    <button onClick={onClick}>
+      <svg viewBox='0 0 35 35' version='1.1' fill=''><g id='' fill=''><path d='M 16 3 C 8.832031 3 3 8.832031 3 16 C 3 23.167969 8.832031 29 16 29 C 23.167969 29 29 23.167969 29 16 C 29 8.832031 23.167969 3 16 3 Z M 16 5 C 22.085938 5 27 9.914063 27 16 C 27 22.085938 22.085938 27 16 27 C 9.914063 27 5 22.085938 5 16 C 5 9.914063 9.914063 5 16 5 Z M 15 10 L 15 15 L 10 15 L 10 17 L 15 17 L 15 22 L 17 22 L 17 17 L 22 17 L 22 15 L 17 15 L 17 10 Z ' fill='' /></g></svg>


When you look at the webpack config, you see an entry for loading svg files with raw loader. so you are able to do:

<div dangerouslySetInnerHTML={{ __html: require("../path/to/assets/icon.svg")}} />

Then you have the possibility to reuse your icons without copying the content of every svg

christianbirg · 2018-06-27T09:18:54Z

src/app/components/Columns/BookmarkAddColumn.js

+  </div>
+)
+
+export default styled(BookmarkAddColumn)`


You misunderstood the concepts of styled components. The right way to use it e.g.

const Input = styled.input` /* some css */ ` const UsecaseSpecificWrapper = styled.div` /* some css */ ${Input} { /* manipulate the css for specific usecase } ` const Usecase => () => { return ( <UsecaseSpecificWrapper> <Input type="text" /> </UsecaseSpecificWrapper> ) }

christianbirg · 2018-06-27T09:20:26Z

src/app/components/Columns/BookmarkCard.js

+)
+
+export default styled(BookmarkCard)`
+  display: ${props => props.dragging ? '0.5' : '1'};


display has no numeric values 💃

christianbirg · 2018-06-27T09:22:38Z

src/app/components/Columns/BookmarkCard.js

+
+export default styled(BookmarkCard)`
+  display: ${props => props.dragging ? '0.5' : '1'};
+  opacity: ${props => props.dragging ? '0' : '1'};


its hard too read. Instead you could write:

export default styled(BookmarkCard)` display: block; opacity: 1; ${props => props.dragging && ` display: none; opacity: 0.5; `} /* ....*/ `

christianbirg · 2018-06-27T09:31:29Z

src/app/containers/Columns/DndBookmarkColumnCover.js

+}
+
+@connect(mapStateToProps)
+export default class DndBookmarkColumnCover extends Component {


In 90% of the cases when you dont implement an shouldComponentUpdate, please use the React.PureComponent, this checks the quality of the props and the new props in the render lifecycle to check if he should render itself or not.

christianbirg · 2018-06-27T09:32:34Z

src/app/index.js

@@ -9,7 +9,7 @@ import { routes } from './routes'

 import './global'

-class Application extends React.PureComponent<{||}> {
+class Application extends Component {


You are right, a PureComponent on a first component make no sense 👌

christianbirg · 2018-06-27T09:36:28Z

src/app/containers/DropDown/DropDownInfoString.js

@@ -0,0 +1,19 @@
+import React, { Component } from 'react'


Thats not wrong, but i dont like the style to import the default and a named export

more beautiful is

import React from 'react' export class Component extends React.Component

or

import { default as React, Component } from 'react'

funny fact: no one knows, that default as is possible 🤓

christianbirg · 2018-06-27T09:44:37Z

src/app/components/Columns/BookmarkColumn.js

+import DndBookmarkColumnCover from '../../containers/Columns/DndBookmarkColumnCover'
+
+// Visualisation of the column for bookmark cards
+const BookmarkColumn = ({ className, id, name, cards }) => (


Functional components doesnt give you the opportunity to implement a shouldComponentUpdate. So everytime the parent is updating, this bookmark component is updating too.

As long React doesnt consider shouldComponentUpdate for functional components, you should use functional components only for displaying ui-data. But its more usecase specific

christianbirg

That is the first review it is only a code review (without checking performance etc)

christianbirg · 2018-06-27T09:49:06Z

src/app/components/BookmarkApp.js

+import ShowBookmarkContainer from '../containers/Columns/ShowBookmarkContainer'
+
+const BookmarkApp = () => (
+  <div>


<React.Fragment />?

christianbirg · 2018-06-27T09:52:31Z

src/app/containers/Columns/ShowBookmarkCard.js

+              updatedAt={wasUpdated(itemList[card].updated_at)}
+              stars={showStars(itemList[card].stargazers_count)}
+              issues={showIssues(itemList[card].open_issues)}
+              onClick={(e) => this.handleDeleteBookmark(e)}


in the constructor you binded the function. Why are you creating a new anonymous function?

christianbirg · 2018-06-27T09:53:12Z

src/app/containers/Columns/ShowBookmarkColumnHead.js

+  render () {
+    return (
+      <BookmarkColumnHead name={this.props.name}
+        onClick={(e) => this.handleDeleteColumn(e)}


same here, anonymous function is unessary

christianbirg · 2018-06-27T09:53:31Z

src/app/containers/DropDown/ShowDropDownButton.js

+
+  render () {
+    return (
+      <DropDownButton onClick={(e) => this.handleClickButton(e)}


christianbirg · 2018-06-27T09:55:47Z

src/app/additional/additionalFunc.js

@@ -0,0 +1,81 @@
+// Creating update info


the naming is a lil bit awkward

good conventionell namings are src/app/utils.js or src/app/utils/helpers.js

christianbirg · 2018-06-27T10:05:25Z

src/app/global.js

 import { injectGlobal } from 'styled-components'

 injectGlobal`
  @import url('https://fonts.googleapis.com/css?family=Open+Sans:300,400');

-  * {


why do you remove this? box-sizing: border-box; is everytime a good thing 🤓

christianbirg · 2018-06-27T10:08:16Z

src/app/containers/Search/ShowSearchInput.js

+  // Handling changing of search input value
+  handleOnChange (e) {
+    const { dispatch } = this.props
+    // if the value has more than 2 symbols


const { value } = e.target

christianbirg · 2018-06-27T10:09:09Z

src/app/containers/Search/ShowSearchInput.js

+    return (
+      <div>
+        <SearchInput
+          value={this.props.searchtext}


searchText ;-)

christianbirg · 2018-06-27T10:11:55Z

src/app/containers/Columns/ShowBookmarkCard.js

+    const itemList = this.props.list
+    const card = this.props.card
+    const { connectDragSource, connectDropTarget, isDragging } = this.props
+


const updatedAt = wasUpdated(itemList[card].updated_at)

Everything is correct. We prefer executing functions not by passing it in a prop of a component

christianbirg · 2018-06-27T10:17:52Z

src/app/containers/Columns/ShowBookmarkAddColumn.js

+    columns: state.bookmarks.columns }
+}
+
+@connect(mapStateToProps)


in the second paramater of connect you can pass an object of actions called mapDispatchToProps

// ... const mapDispatchToProps = { addColumn } @connect(mapStateToProps, mapDispatchToProps) class Component from React.Component { /* ... */ } /* somewhere in the component */ this.props.addColumn()

so you call the function as a property of a component, but he will execute the function over store.dispatch

because the task

ccbcc72

it was interesting

christianbirg reviewed Jun 27, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

because the task #1

because the task #1

trypolski commented Jun 12, 2018

christianbirg Jun 27, 2018

christianbirg Jun 27, 2018 •

edited

Loading

christianbirg Jun 27, 2018 •

edited

Loading

christianbirg Jun 27, 2018

christianbirg Jun 27, 2018

christianbirg Jun 27, 2018

christianbirg Jun 27, 2018 •

edited

Loading

christianbirg Jun 27, 2018

christianbirg Jun 27, 2018

christianbirg left a comment

christianbirg Jun 27, 2018

christianbirg Jun 27, 2018 •

edited

Loading

christianbirg Jun 27, 2018

christianbirg Jun 27, 2018

christianbirg Jun 27, 2018

christianbirg Jun 27, 2018

christianbirg Jun 27, 2018

christianbirg Jun 27, 2018

christianbirg Jun 27, 2018

christianbirg Jun 27, 2018 •

edited

Loading

because the task #1

Are you sure you want to change the base?

because the task #1

Conversation

trypolski commented Jun 12, 2018

Choose a reason for hiding this comment

christianbirg Jun 27, 2018 • edited Loading

Choose a reason for hiding this comment

christianbirg Jun 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christianbirg Jun 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christianbirg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christianbirg Jun 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christianbirg Jun 27, 2018 • edited Loading

Choose a reason for hiding this comment

christianbirg Jun 27, 2018 •

edited

Loading

christianbirg Jun 27, 2018 •

edited

Loading

christianbirg Jun 27, 2018 •

edited

Loading

christianbirg Jun 27, 2018 •

edited

Loading

christianbirg Jun 27, 2018 •

edited

Loading