Adds React/Redux example #1706

paguillama · 2016-10-17T17:46:40Z

It is basically this React/Redux example, without some non-react-redux code, and some changes to match the specs.

paguillama · 2016-10-18T00:19:59Z

I will work on the CI issue :)

paguillama · 2016-10-18T05:28:30Z

.jscsrc

@@ -50,6 +50,8 @@
 		"examples/typescript-*/js/**/*.js",
 		"examples/vanilladart/**/*.js",
 		"examples/vanilla-es6/dist/bundle.js",
+		"examples/react-redux/js/reducers/*.js",


These reducers use object spread operator.

Adding the esnext option to jscs works but I couldn't find a way to extend a jscs configuration or use a specific one on the js folder in order to override this option. I also tried disabling jscs with inline comments (/* jscs: disable */) and didn't work. So I removed reducers from linting :|

Check http://stackoverflow.com/questions/25223149/is-there-any-way-for-jscs-to-ignore-rules-per-file-block-or-line

Quoting:
To ignore all rules

// Code here will be linted with JSCS.
// jscs:disable
// Code here will be ignored by JSCS.
// jscs:enable
To ignore a specific rule:

// Code here will be linted with JSCS.
// jscs:disable specificRule
// Code here will be ignored by JSCS.
// jscs:enable specificRule

I tried it, but it didn't work. The only way I found to make it work is with the esnext flag.

paguillama · 2016-10-18T06:07:49Z

Test output:

bash ./test-runner.sh 

> [email protected] test-server /home/gablo/Desktop/projects/pg/todomvc
> gulp test-server &

[02:57:27] Using gulpfile ~/Desktop/projects/pg/todomvc/gulpfile.js
[02:57:27] Starting 'test-server'...
[02:57:27] Finished 'test-server' after 3.67 ms

> @ lint /home/gablo/Desktop/projects/pg/todomvc/tooling
> cd .. && jscs "examples/react-redux" "tests/"

react-redux, NaN, NaN, NaN

> todomvc-browser-tests@ test /home/gablo/Desktop/projects/pg/todomvc/tests
> mocha allTests.js --no-timeouts --reporter spec "--framework=react-redux"



  TodoMVC - react-redux
    When page is initially opened
      ✓ should focus on the todo input field
    No Todos
      ✓ should hide #main and #footer (315ms)
    New Todo
      ✓ should allow me to add todo items (329ms)
      ✓ should clear text input field when an item is added (194ms)
      ✓ should append new items to the bottom of the list (495ms)
      ✓ should trim text input (187ms)
      ✓ should show #main and #footer when items added (199ms)
    Mark all as completed
      ✓ should allow me to mark all items as completed (67ms)
      ✓ should correctly update the complete all checked state (185ms)
      ✓ should allow me to clear the completion state of all items (91ms)
      ✓ complete all checkbox should update state when items are completed / cleared (182ms)
    Item
      ✓ should allow me to mark items as complete (399ms)
      ✓ should allow me to un-mark items as complete (383ms)
    Editing
      ✓ should focus the input (38ms)
      ✓ should hide other controls when editing (130ms)
      ✓ should save edits on enter (220ms)
      ✓ should save edits on blur (274ms)
      ✓ should trim entered text (238ms)
      ✓ should remove the item if an empty text string was entered (172ms)
      ✓ should cancel edits on escape (196ms)
    Counter
      ✓ should display the current number of todo items (304ms)
    Clear completed button
      ✓ should display the correct text (86ms)
      ✓ should remove completed items when clicked (204ms)
      ✓ should be hidden when there are no items that are completed (227ms)
    Persistence
      ✓ should persist its data (670ms)
    Routing
      ✓ should allow me to display active items (181ms)
      ✓ should respect the back button (353ms)
      ✓ should allow me to display completed items (235ms)
      ✓ should allow me to display all items (325ms)
      ✓ should highlight the currently applied filter (164ms)


  30 passing (55s)

arthurvr · 2016-11-06T14:43:37Z

Took a quick look and the code looks very good, except some tiny style issues. I personally don't use Redux or React frequently though. Do you know anyone active in the redux community that might want to review this?

Thanks for the PR!

paguillama · 2016-11-07T21:22:07Z

Yes, maybe @fkereki can help us reviewing this PR

fkerekiglobant · 2016-11-10T19:25:30Z

examples/react-redux/js/actions/actionTypes.js

@@ -0,0 +1,7 @@
+// Action types constants
+export const ADD_TODO = 'ADD_TODO';


I'd suggest alphabetic ordering.

fkerekiglobant · 2016-11-10T19:28:27Z

examples/react-redux/js/components/AddTodo.jsx

+
+export default class AddTodo extends React.Component {
+
+	static propTypes = {


Check http://stackoverflow.com/questions/38363156/static-proptypes-not-working-under-es6 -- is the static usage valid?

Removing the actions property throws:

Warning: Failed prop type: Required prop `actions` was not specified in `AddTodo`. in AddTodo (created by Connect(AddTodo)) in Connect(AddTodo) (created by Header) in header (created by Header) in Header (created by App) in div (created by App) in App (created by Connect(App)) in Connect(App) (created by RouterContext) in RouterContext (created by Router) in Router in Provider

That's because the stage-0 preset contains the transform-class-properties, transpiling static properties for us.

fkerekiglobant · 2016-11-10T19:29:20Z

examples/react-redux/js/components/AddTodo.jsx

+
+	static ENTER_KEY = 13
+
+	constructor() {


Usual is

constructor(props){ super(props);

fkerekiglobant · 2016-11-10T19:31:03Z

examples/react-redux/js/components/Footer.jsx

+		todos: PropTypes.array.isRequired
+	}
+
+	constructor() {


Same comment as above regarding props parameter

fkerekiglobant · 2016-11-10T19:33:09Z

examples/react-redux/js/components/Footer.jsx

+					</li>
+				</ul>
+				{
+					!completedCount ?


Generally you prefer assigning the expression to a variable, and here just write {thatVariable}

fkerekiglobant · 2016-11-10T19:34:46Z

examples/react-redux/js/components/Header.jsx

+const Header = () => (
+	<header className="header">
+		<h1>todos</h1>
+		<AddTodo></AddTodo>


Usually you would write <AddTodo/>

fkerekiglobant · 2016-11-10T19:36:45Z

examples/react-redux/js/components/Todo.jsx

+		text: PropTypes.string.isRequired
+	}
+
+	static ENTER_KEY = 13


Better than statics, use a constants.js file

fkerekiglobant · 2016-11-10T19:37:50Z

examples/react-redux/js/components/Todo.jsx

+	}
+
+	render() {
+		const label = this.state.editing ?


I'd rather have an if (this.state.editing) and assign variables within

fkerekiglobant · 2016-11-10T19:38:30Z

examples/react-redux/js/components/Todo.jsx

+				type="text"
+				 value={this.state.newText}
+				 ref={input => {
+					 if (input != null) {


You can write input && input.focus()

fkerekiglobant · 2016-11-10T19:39:02Z

examples/react-redux/js/components/Todo.jsx

+				 onChange={this.handleEditChange}></input>;
+
+		let classNames = [];
+		if (this.props.completed) {


You can write (this.props.completed) && classNames.push("completed");

fkerekiglobant · 2016-11-10T19:39:24Z

examples/react-redux/js/components/TodoList.jsx

+		}).isRequired).isRequired
+	}
+
+	constructor() {


See comment above

fkerekiglobant · 2016-11-10T19:40:16Z

examples/react-redux/js/components/TodoList.jsx

+			/>
+		);
+
+		const allCompleted = this.props.todos


Maybe do a find to see if there's any non-completed one?

fkerekiglobant · 2016-11-10T19:41:02Z

examples/react-redux/js/index.jsx

+// Renders the application on the .todoapp element
+render(
+	<Provider store={store}>
+		{/* Provider passes the store to child elements through React context


Are these comments needed?

I know it's too much for real code, but they will give others a better understanding on what's going on in order to take a good decision about what framework to use.

fkerekiglobant · 2016-11-10T19:41:49Z

examples/react-redux/js/reducers/todo.js

+
+const toggleTodo = state => {
+	return {
+		// Object spread operator is not supported by jshint yet


Check -- does this work?

Yes, jshint ignore works fine ignoring the spread operator. jscs: disable is the breaking one.

fkerekiglobant · 2016-11-10T19:42:26Z

examples/react-redux/js/reducers/todo.js

+		case actionTypes.TOGGLE_TODO:
+			if (state.id !== action.id) {
+				return state;
+			}


I'd prefer an else here

fkerekiglobant · 2016-11-10T19:43:04Z

examples/react-redux/js/reducers/todo.js

+
+			return toggleTodo(state);
+		case actionTypes.TOGGLE_ALL:
+			return action.toggleTo === state.completed ? state : toggleTodo(state);


Use the same style in the previous case, and this one; either an if/then/else or a ternary ? operator.

fkerekiglobant · 2016-11-10T19:43:27Z

examples/react-redux/js/storage.js

+		// so reducers will use their own defaults
+		if (serializedState === null) {
+			return undefined;
+		}


I'd include an else here.

fkerekiglobant

Please look at the comments, OK?

Code changes regarding review comments

paguillama · 2017-01-06T19:13:05Z

Happy new year community!!! As usual, people try to remove past year pending stuff, and I'm not the exception :) Someone else wants to help reviewing this PR?

FadySamirSadek · 2023-09-20T03:59:27Z

Dear @paguillama,

I sincerely apologize for the delay in addressing your contribution. The project experienced a period of inactivity, and combined with my personal time constraints, several updates, including yours, remained unaddressed for longer than anticipated. I am actively working to remedy these outstanding tasks and am committed to ensuring more timely responses in the future.

Regrettably, I must close this PR due to its age and extended period without updates. However, if you are still keen on aiding the effort to publish the react-redux example on the TodoMVC website, I would greatly appreciate your expertise. Please consider reviewing PR #2198 and offering any suggestions you believe might enhance the implementation.

Thank you for your understanding and continued support.

Adds React/Redux example

644ec09

paguillama added 4 commits October 18, 2016 02:13

Uses hashHistory on router and hides footer and main when no todos

466e103

Adds React + Redux to learn.json

816c15d

Uses react-router v3.0.0-beta.1

e714c96

Excludes dist and reducers from jscs

4abf220

paguillama commented Oct 18, 2016

View reviewed changes

Fixes pluralization and todo edition

2b7af41

fkerekiglobant reviewed Nov 10, 2016

View reviewed changes

examples/react-redux/js/components/AddTodo.jsx

static ENTER_KEY = 13

constructor() {

Copy link

fkerekiglobant Nov 10, 2016

Choose a reason for hiding this comment

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

Usual is

constructor(props){ super(props);

fkerekiglobant reviewed Nov 10, 2016

View reviewed changes

examples/react-redux/js/components/TodoList.jsx

}).isRequired).isRequired

}

constructor() {

Copy link

fkerekiglobant Nov 10, 2016

Choose a reason for hiding this comment

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

See comment above

fkerekiglobant reviewed Nov 10, 2016

View reviewed changes

[Review comments]

6e63851

Code changes regarding review comments

fkerekiglobant approved these changes Dec 1, 2016

View reviewed changes

FadySamirSadek closed this Sep 20, 2023

FadySamirSadek mentioned this pull request Sep 20, 2023

Redux implementation #1674

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds React/Redux example #1706

Adds React/Redux example #1706

paguillama commented Oct 17, 2016 •

edited

Loading

paguillama commented Oct 18, 2016

paguillama Oct 18, 2016 •

edited

Loading

fkerekiglobant Nov 10, 2016

paguillama Nov 15, 2016

paguillama commented Oct 18, 2016

arthurvr commented Nov 6, 2016

paguillama commented Nov 7, 2016

fkerekiglobant Nov 10, 2016

paguillama Nov 15, 2016

fkerekiglobant Nov 10, 2016

paguillama Nov 15, 2016

fkerekiglobant Nov 10, 2016

fkerekiglobant Nov 10, 2016

fkerekiglobant Nov 10, 2016

fkerekiglobant Nov 10, 2016

fkerekiglobant Nov 10, 2016

fkerekiglobant Nov 10, 2016

fkerekiglobant Nov 10, 2016

fkerekiglobant Nov 10, 2016

fkerekiglobant Nov 10, 2016

fkerekiglobant Nov 10, 2016

fkerekiglobant Nov 10, 2016

paguillama Nov 15, 2016 •

edited

Loading

fkerekiglobant Nov 10, 2016

paguillama Nov 15, 2016

fkerekiglobant Nov 10, 2016

fkerekiglobant Nov 10, 2016

fkerekiglobant Nov 10, 2016

fkerekiglobant left a comment

paguillama commented Jan 6, 2017

FadySamirSadek commented Sep 20, 2023

		@@ -0,0 +1,7 @@
		// Action types constants
		export const ADD_TODO = 'ADD_TODO';


		export default class AddTodo extends React.Component {

		static propTypes = {

Adds React/Redux example #1706

Adds React/Redux example #1706

Conversation

paguillama commented Oct 17, 2016 • edited Loading

paguillama commented Oct 18, 2016

paguillama Oct 18, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paguillama commented Oct 18, 2016

arthurvr commented Nov 6, 2016

paguillama commented Nov 7, 2016

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

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

paguillama Nov 15, 2016 • 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

fkerekiglobant left a comment

Choose a reason for hiding this comment

paguillama commented Jan 6, 2017

FadySamirSadek commented Sep 20, 2023

paguillama commented Oct 17, 2016 •

edited

Loading

paguillama Oct 18, 2016 •

edited

Loading

paguillama Nov 15, 2016 •

edited

Loading