create react sample application #42

DanaShalev · 2018-01-14T13:37:37Z

Create react sample application over cloudinary react sdk

eitanp461 · 2018-01-23T15:02:07Z

samples/photo_album/config/webpack.config.dev.js

@@ -0,0 +1,264 @@
+'use strict';


Do you remember why you needed to eject Webpack's config?
Could the sample app work without ejecting the config file?
If not, please highlight in the README.md the reasons for ejecting Webpack config in this sample

eitanp461 · 2018-01-23T15:03:47Z

samples/photo_album/public/index.html

+    <noscript>
+      You need to enable JavaScript to run this app.
+    </noscript>
+    <script src="//widget.cloudinary.com/global/all.js" type="text/javascript"></script>


Add a comment to the README about the different upload options and how to configure them, add a comment here about why this file is optionally included

eitanp461 · 2018-01-23T15:23:09Z

samples/photo_album/src/Cloudinary.js

+
+export default class Cloudinary {
+
+    static url(publicId, options) {


Why do you need a class that has only static methods?
Why not export the functions independently and let consumers import only what they need?

Then you can rename the file to CloudinaryService and use it as

import { url } from './CloudinaryService.js'

eitanp461 · 2018-01-23T15:26:05Z

samples/photo_album/src/actions/index.js

@@ -0,0 +1,54 @@
+import Cloudinary from "../Cloudinary";


Code style isn't consistent. Some files import with a single quote, others with double quotes.
What about adding a .editorconfig file for consistency?
And maybe use prettier too?

eitanp461 · 2018-01-23T15:28:24Z

samples/photo_album/src/actions/index.js

+    publicId: publicId
+});
+
+export const fetchPhotos = (cloudName) => {


This shouldn't be in the actions file, it's a utility method. It should move the CloudinaryService.js (the name I proposed a few comments ago)

eitanp461 · 2018-01-23T16:14:03Z

samples/photo_album/src/reducers/PhotosListReducer.js

+            const index = photos.findIndex(current => current.public_id === action.publicId);
+            return [...photos.slice(0, index), ...photos.slice(index + 1)];
+        default:
+            return photos;


Should return an immutable object

eitanp461 · 2018-01-23T16:17:11Z

samples/photo_album/src/reducers/UploadedPhotosReducer.js

+            return [...uploadedPhotos.slice(0, index), ...uploadedPhotos.slice(index + 1)];
+        }
+        default:
+            return uploadedPhotos;


Should return an immutable object

eitanp461 · 2018-01-23T16:17:34Z

samples/photo_album/src/reducers/UploadedPhotosReducer.js

+                uploadedPhotos.map(current => current.id === newPhoto.id ? {...current, ...newPhoto} : current);
+        }
+        case 'DELETE_UPLOADED_PHOTO': {
+            const index = uploadedPhotos.findIndex(current => current.response.body.public_id === action.publicId);


Use Array.filter

eitanp461 · 2018-01-23T16:18:16Z

samples/photo_album/src/reducers/UploadedPhotosReducer.js

+
+            return (index === -1) ?
+                [newPhoto, ...uploadedPhotos] :
+                uploadedPhotos.map(current => current.id === newPhoto.id ? {...current, ...newPhoto} : current);


You already have the index of the changed item, why iterate again, and not change that index?

eitanp461 · 2018-01-23T16:19:21Z

samples/photo_album/README.md

@@ -0,0 +1,2229 @@
+This project was bootstrapped with [Create React App](https://github.com/facebookincubator/create-react-app).


Add to the beginning of this generated README a description of the functionality of this sample and any other important information the reader might need

eitanp461 · 2018-07-17T12:38:17Z

samples/photo_album/src/components/PhotosUploader.js

+import { NavLink } from 'react-router-dom';
+import { connect } from 'react-redux';
 import request from 'superagent';
 import Dropzone from 'react-dropzone';


Package is not listed in package.json/yarn.lock

eitanp461 · 2018-07-17T12:39:43Z

samples/photo_album/src/components/UploadedPhotosStatus.js

 import PropTypes from 'prop-types';
-import {connect} from "react-redux";
+import { connect } from 'react-redux';
 import request from 'superagent';


Package is not listed in package.json/yarn.lock

eitanp461 · 2018-07-17T12:40:52Z

samples/photo_album/src/components/PhotoList.js

+                    Take a look at our documentation of{' '}
+                    <a
+                        href="https://cloudinary.com/documentation/image_transformations"
+                        target="_blank"


Error in console - Line 51: Using target="_blank" without rel="noopener noreferrer" is a security risk: see https://mathiasbynens.github.io/rel-noopener react/jsx-no-target-blank

eitanp461 · 2018-07-17T12:42:11Z

samples/photo_album/src/index.js

 render(
    <Provider store={store}>
-        <AppContainer cloudName='dvxpdqpjj' uploadPreset='hvs8orho'/>
+        <AppContainer cloudName="danashalev" uploadPreset="hvs8orho" />


This shouldn't have your credentials. Read cloud name + upload preset from a config file provided by the user (you can create a sample config file with placeholders).

eitanp461 · 2018-07-17T12:44:25Z

samples/photo_album/src/components/Introduction.js

-                                    y='12'>
-                    </Transformation>
+                <Video
+                    sourceTypes={'webm'}


Why limit to webm? The video can't play on Safari

eitanp461 · 2018-07-17T12:49:09Z

samples/photo_album/src/reducers/PhotosListReducer.js

+const PhotosListReducer = (photos = [], action) => {
+    switch (action.type) {
+        case 'PHOTOS_FETCHED':
+            return action.photos;


This comment was probably overlooked. The returned object should be immutable (at least return a copy of action.photos)

eitanp461

Added one more comment

eitanp461 · 2018-07-17T12:54:27Z

samples/photo_album/src/components/App.js

+};
+
+const AppContainer = connect(
+    () => {},


This function should return an object. Creates the following error in browser console:
mapStateToProps() in Connect(App) must return a plain object. Instead received undefined.

create react sample application

4e0f348

DanaShalev assigned eitanp461 Jan 14, 2018

DanaShalev requested a review from eitanp461 January 14, 2018 13:37

use new react-sdk and fix imports

312ff8e

eitanp461 requested changes Jan 23, 2018

View reviewed changes

PR comments fixes

f83e4e5

eitanp461 reviewed Jul 17, 2018

View reviewed changes

eitanp461 assigned DanaShalev100 and unassigned eitanp461 Jul 17, 2018

eitanp461 reviewed Jul 17, 2018

View reviewed changes

PR comments fixes

069c957

DanaShalev100 assigned eitanp461 and unassigned DanaShalev100 Jul 17, 2018

PR comments fixes

b7a1b43

DanaShalev100 assigned DanaShalev100 and eitanp461 and unassigned eitanp461 and DanaShalev100 Jul 18, 2018

PR comments fixes

6458129

DanaShalev100 assigned eitanp461 and unassigned eitanp461 Jul 18, 2018

PR comments fixes

2fa0ca6

eitanp461 approved these changes Jul 18, 2018

View reviewed changes

eitanp461 assigned DanaShalev100 and unassigned eitanp461 Jul 18, 2018

DanaShalev100 merged commit 3d7c2aa into master Jul 18, 2018

DanaShalev100 deleted the react-sample branch July 18, 2018 09:30


		export default class Cloudinary {

		static url(publicId, options) {

		@@ -0,0 +1,2229 @@
		This project was bootstrapped with [Create React App](https://github.com/facebookincubator/create-react-app).

create react sample application #42

create react sample application #42

Uh oh!

Conversation

DanaShalev commented Jan 14, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eitanp461 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants