Complete app's logic #1

Fafruch · 2017-10-14T20:39:41Z

This PR contains following:

visual representation of all possible options to choose from (no CSS - TO DO)
add handling rrule.js converting library with users input from components
add rrule.js, moment.js, lodash, react-datetime, react-copy-to-clipboard dependencies

ar-mac

Not all changes are required to be applied.
It's just opening of discussion

ar-mac · 2017-10-14T20:50:42Z

src/App.js

-    <p className="App-intro">
-      To get started, edit <code>src/App.js</code> and save to reload.
-    </p>
+  <div>


Obsolete divs

ar-mac · 2017-10-14T20:55:53Z

src/components/AppContainer.js

@@ -0,0 +1,111 @@
+import React, { Component } from 'react';
+import _ from 'lodash';


Webpack 2 provides tree shaking so unused lodash methods are cut out from the bundle.
However that's not always the case on other projects, so we tent to only import methods we need from lodash.
Please use named imports. This also removes the need of having _. in your code

I wanted to emphasize using lodash'es function (e.g. isNaN(), not the original one). So should I use import isNaN from 'lodash/isNaN'; rather than this approach?

the common practice is
import { isNan, isEmpty, otherMethod } from 'lodash'

ar-mac · 2017-10-14T20:56:50Z

src/components/AppContainer.js

+  constructor(props) {
+    super(props);
+
+    this.state = {


(minor) as state does not use props, then it can be defined outside constructor.

Also binding can be omitted (next comment)

Hmm, when trying to move it outside the constructor I got the following error when I hover 'state':
Conflicting inherited declaration React.Component.state was found in namespace public.

I see I got in emoji_roulette state in the way you described and it wasn't treated as an error. What's interesting, { Component } imported from react has there purple color while in rrule project it's white.

There is and eslint error (unexpected token =) but this is actually working. And might be the issue with eslint itselft babel/babel-eslint#312
This is stylistic suggestion so there is no need to change that.

In emoji_roulette we have React 15.6.1 while here you have 16.0.0. This might be the case that WebStorm does not know where to find import { Component }

According to state - it's webstorm related (not ESLint error)

ar-mac · 2017-10-14T20:57:48Z

src/components/AppContainer.js

+    this.handleCopy = this.handleCopy.bind(this);
+  }
+
+  handleChange(event) {


What about using handleChange = (event) => { that way you do not need to this.handleChange = this.handleChange.bind(this);

What to heck, again I got an error. This time is ESLint: Parsing error: Unexpected token = on the first '='. It looks like more global Component problem??

EDIT. But it do work
EDIT 2. Adding "parser": "babel-eslint" to .eslintrc fixed that

ar-mac · 2017-10-14T21:01:51Z

src/components/AppContainer.js

+  handleChange(event) {
+    const newData = _.cloneDeep(this.state.data);
+    _.set(newData, event.target.name, event.target.value);
+    this.setState({ data: newData, isCopied: false });


During coder dojo we found bug in this solution when handleChange gets called faster than new state value is accessible in state.
the fix was:

handleChange = (event) => { this.setState((currentState)=> { const newData = cloneDeep(currentState.data); set(newData, event.target.name, event.target.value); return { data: newData, isCopied: false }; }) }

This change seems to require event.persist(); in all onChange functions. Ugh :(
Is it critical? Or how often did it happen to you (or in which situations)?

This issue happened when we had two fields dependent on each other and when one was changed, we immediately called the change handle for the second field.

Regarding this event.persist();
You can call event.persist on the beginning of this handleChange method, and not in every onChange function.

ar-mac · 2017-10-14T21:31:34Z

src/components/Repeat/Monthly/On.js

+        handleChange(editedEvent);
+      }}
+    >
+      {[...new Array(31)].map((day, i) => <option key={i} value={i + 1}>{i + 1}</option>)}


good one 👍

ar-mac · 2017-10-14T21:46:16Z

src/components/Repeat/Weekly/index.js

+
+    <div className="btn-group" data-toggle="buttons">
+      {_.toPairs(days).map((day) => {
+        const dayName = day[0];


what about using destructuring for that?

ar-mac · 2017-10-14T21:58:13Z

src/components/Repeat/Yearly/On.js

+      className="form-control"
+      checked={mode === 'on'}
+      onChange={(event) => {
+        const editedEvent = { ...event, target: { ...event.target, value: 'on', name: event.target.name } };


what about adding value="on" to the input field?
That way you will not have to edit event.
Here and in other cases

Yeah, didn't know I can do it like that. Thanks

ar-mac · 2017-10-14T22:04:07Z

src/utils/computeWeekly.js

+  };
+
+  const activeDays = [];
+  _.values(days).forEach((isDayActive, dayIndex) => {


This is perfect use-case for reduce method

ar-mac · 2017-10-14T22:05:44Z

src/utils/computeYearlyOn.js

+const computeYearlyOn = (on) => {
+  const repeat = {};
+
+  months.forEach((month, monthIndex) => {


another use-case for reduce

Fafruch · 2017-10-15T20:57:13Z

src/utils/computeYearlyOn.js

-};
+const computeYearlyOn = on => ({
+  bymonth: months.reduce(
+    (lastMonth, month, monthIndex) =>


Didn't know how to name first argument

Ahhh sorry I did not understood the intention of what this code was supposed to accomplish.
Using reduce is not the correct tool here, as you want to get the index of on.month from array of months and add 1 to it.

This can be done much easier by bymonth: months.indexOf(on.month) + 1,

Fafruch added 18 commits October 8, 2017 20:41

Create basic components with state and its handlers, configure eslint

e655dd3

Add bootstrap dependency and basic functionality

35a48c4

End component has now conditional render

f00dcb0

Add comments in endAfter form

fe61973

Remove unnecessary <form> tags from End

4980a5f

Generating ending rrule works, add rrule.js & moment.js dependencies

342043e

Add html representation in Repeat

dae53d1

Repeat component and its children is finished (despite CSS), add lodash

cb3f538

Converting to rrule works for all options, small fixes in components

5998bfa

Split End component

6cc8f31

Fix mapping in RepeatWeekly

5741ac1

Rebuilt app's state and its handleChange

8eef2b4

Make computeRRule handle new data

d3401ea

Change app's structure and on/onThe state's structure

f2f5205

Split computeRRule function's logic

7b2b5cb

Remove unnecessary spreads from editedEvent objects

346c577

Add react-copy-to-clipboard dependency

a71aecd

Change 'frequency' name to 'interval' to match RRule's name

5f68ea9

ar-mac suggested changes Oct 14, 2017

View reviewed changes

Fix after review

e72dc8a

Fafruch commented Oct 15, 2017

View reviewed changes

Fafruch added 4 commits October 15, 2017 23:40

Fix after review continued

a1bc1a9

Fix handleChange and simplify rrule yearly / bymonth

b38f036

Remove App.css, update package.json

44925fa

Remove obsolete divs in App

5fb221d

Fafruch merged commit ab00ff6 into master Oct 16, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Complete app's logic #1

Complete app's logic #1

Fafruch commented Oct 14, 2017

ar-mac left a comment

ar-mac Oct 14, 2017

ar-mac Oct 14, 2017

Fafruch Oct 15, 2017 •

edited

Loading

ar-mac Oct 15, 2017

ar-mac Oct 14, 2017

Fafruch Oct 15, 2017 •

edited

Loading

ar-mac Oct 15, 2017

ar-mac Oct 15, 2017

Fafruch Oct 15, 2017

ar-mac Oct 14, 2017

Fafruch Oct 15, 2017 •

edited

Loading

ar-mac Oct 14, 2017

Fafruch Oct 15, 2017 •

edited

Loading

ar-mac Oct 15, 2017

ar-mac Oct 14, 2017

ar-mac Oct 14, 2017

Fafruch Oct 15, 2017

ar-mac Oct 14, 2017

Fafruch Oct 15, 2017

ar-mac Oct 14, 2017

ar-mac Oct 14, 2017

Fafruch Oct 15, 2017

ar-mac Oct 15, 2017

		@@ -0,0 +1,111 @@
		import React, { Component } from 'react';
		import _ from 'lodash';

Complete app's logic #1

Complete app's logic #1

Conversation

Fafruch commented Oct 14, 2017

ar-mac left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fafruch Oct 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fafruch Oct 15, 2017 • 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

Fafruch Oct 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fafruch Oct 15, 2017 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fafruch Oct 15, 2017 •

edited

Loading

Fafruch Oct 15, 2017 •

edited

Loading

Fafruch Oct 15, 2017 •

edited

Loading

Fafruch Oct 15, 2017 •

edited

Loading