create react sample application#42
Conversation
| @@ -0,0 +1,264 @@ | |||
| 'use strict'; | |||
There was a problem hiding this comment.
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
| <noscript> | ||
| You need to enable JavaScript to run this app. | ||
| </noscript> | ||
| <script src=/proxy/https/github.com/cloudinary/cloudinary-react/pull/"//widget.cloudinary.com/global/all.js" type="text/javascript"></script> |
There was a problem hiding this comment.
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
|
|
||
| export default class Cloudinary { | ||
|
|
||
| static url(publicId, options) { |
There was a problem hiding this comment.
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'
| @@ -0,0 +1,54 @@ | |||
| import Cloudinary from "../Cloudinary"; | |||
There was a problem hiding this comment.
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?
| publicId: publicId | ||
| }); | ||
|
|
||
| export const fetchPhotos = (cloudName) => { |
There was a problem hiding this comment.
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)
| const index = photos.findIndex(current => current.public_id === action.publicId); | ||
| return [...photos.slice(0, index), ...photos.slice(index + 1)]; | ||
| default: | ||
| return photos; |
There was a problem hiding this comment.
Should return an immutable object
| return [...uploadedPhotos.slice(0, index), ...uploadedPhotos.slice(index + 1)]; | ||
| } | ||
| default: | ||
| return uploadedPhotos; |
There was a problem hiding this comment.
Should return an immutable object
| 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); |
|
|
||
| return (index === -1) ? | ||
| [newPhoto, ...uploadedPhotos] : | ||
| uploadedPhotos.map(current => current.id === newPhoto.id ? {...current, ...newPhoto} : current); |
There was a problem hiding this comment.
You already have the index of the changed item, why iterate again, and not change that index?
| @@ -0,0 +1,2229 @@ | |||
| This project was bootstrapped with [Create React App](https://github.com/facebookincubator/create-react-app). | |||
There was a problem hiding this comment.
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
| import { NavLink } from 'react-router-dom'; | ||
| import { connect } from 'react-redux'; | ||
| import request from 'superagent'; | ||
| import Dropzone from 'react-dropzone'; |
There was a problem hiding this comment.
Package is not listed in package.json/yarn.lock
| import PropTypes from 'prop-types'; | ||
| import {connect} from "react-redux"; | ||
| import { connect } from 'react-redux'; | ||
| import request from 'superagent'; |
There was a problem hiding this comment.
Package is not listed in package.json/yarn.lock
| Take a look at our documentation of{' '} | ||
| <a | ||
| href=/proxy/https/github.com/cloudinary/cloudinary-react/pull/"https://cloudinary.com/documentation/image_transformations"%3C/span> | ||
| target="_blank" |
There was a problem hiding this comment.
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
| render( | ||
| <Provider store={store}> | ||
| <AppContainer cloudName='dvxpdqpjj' uploadPreset='hvs8orho'/> | ||
| <AppContainer cloudName="danashalev" uploadPreset="hvs8orho" /> |
There was a problem hiding this comment.
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).
| y='12'> | ||
| </Transformation> | ||
| <Video | ||
| sourceTypes={'webm'} |
There was a problem hiding this comment.
Why limit to webm? The video can't play on Safari
| const PhotosListReducer = (photos = [], action) => { | ||
| switch (action.type) { | ||
| case 'PHOTOS_FETCHED': | ||
| return action.photos; |
There was a problem hiding this comment.
This comment was probably overlooked. The returned object should be immutable (at least return a copy of action.photos)
eitanp461
left a comment
There was a problem hiding this comment.
Added one more comment
| }; | ||
|
|
||
| const AppContainer = connect( | ||
| () => {}, |
There was a problem hiding this comment.
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 over cloudinary react sdk