roger - no ticket - Multiple file upload by roggc · Pull Request #428 · cheapreats/react-ui-library

roggc · 2022-01-02T12:11:14Z

multiple file upload in parallel

ralph-dev · 2022-01-02T12:32:06Z

Two things

I should be able to see both files uploaded at the same time (Progress of Each individually) because it looked liked the second file didn't get uploaded until it suddenly appeared
Can't we upload two simultaneously?

ralph-dev · 2022-01-02T12:33:57Z

src/Containers/FileUpload/reducer.ts

+}
+
+export interface IFileUploadState {
+    height: number | undefined;


Do you mean optional instead of undefined?

ralph-dev · 2022-01-02T12:34:25Z

src/Containers/FileUpload/reducer.ts

+        case SET_INITIAL_HEIGHT_PLUS_VALUES:
+            return {
+                ...state,
+                height: undefined,


Isn't this just default height? Or don't set it at all?

roggc · 2022-01-02T12:38:58Z

Two things

I should be able to see both files uploaded at the same time (Progress of Each individually) because it looked liked the second file didn't get uploaded until it suddenly appeared

Can't we upload two simultaneously?

so you mean to process files in parallel?

ralph-dev · 2022-01-02T12:39:40Z

Two things

I should be able to see both files uploaded at the same time (Progress of Each individually) because it looked liked the second file didn't get uploaded until it suddenly appeared

Can't we upload two simultaneously?

so you mean to process files in parallel?

Correct. No need to do step by step.

ralph-dev

Code quality

src/Containers/FileUpload/FileUpload.tsx

ralph-dev · 2022-01-03T16:05:28Z

src/Containers/FileUpload/FileUpload.tsx

+                                return {
+                                    ...value_,
+                                    isSuccess: {
+                                        ...value_.isSuccess,
+                                        value: false,
+                                    },
+                                    isFailure: {
+                                        ...value_.isFailure,
+                                        value: true,
+                                    },
+                                    isUploading: {
+                                        ...value_.isUploading,
+                                        value: false,
+                                    },


Duplicate code with piece below abstract to function to reduce code duplicate. Use parameters for function to solve issue

ralph-dev · 2022-01-03T16:05:43Z

src/Containers/FileUpload/FileUpload.tsx

+                                    ...value_,
+                                    isSuccess: {
+                                        ...value_.isSuccess,
+                                        value: true,
+                                    },
+                                    isFailure: {
+                                        ...value_.isFailure,
+                                        value: false,
+                                    },
+                                    isUploading: {
+                                        ...value_.isUploading,
+                                        value: false,
+                                    },


Duplicate as above

src/Containers/FileUpload/FileUpload.tsx

ralph-dev · 2022-01-03T16:07:18Z

src/Containers/FileUpload/FileUpload.tsx

+                    <IsFailureIsSuccessPanel
+                        message={failureMessage}
+                        iconColor={MainTheme.colors.statusColors.red}
+                        IconToShow={TimesCircle}
+                        transitionDuration={animationDuration}
+                    />
+                );
+            }
+            if (value.isSuccess.value) {
+                return (
+                    <IsFailureIsSuccessPanel
+                        message={successMessage}
+                        iconColor={MainTheme.colors.statusColors.green}
+                        IconToShow={CheckCircle}
+                        transitionDuration={animationDuration}
+                    />


Use one Return Statement and just edit the Props Object since duplicate JSX

ralph-dev · 2022-01-03T16:08:33Z

src/Containers/FileUpload/FileUpload.tsx

+                    opacity={
+                        value.isUploading.opacity ||
+                        value.isSuccess.opacity ||
+                        value.isFailure.opacity
+                    }
+                    position={
+                        value.isUploading.isPosition &&
+                        value.isFailure.isPosition &&
+                        value.isSuccess.isPosition
+                    }


Do

Const {isFailure, IsSuccees} = value
So we don't right value. 100 times

ralph-dev · 2022-01-03T21:12:23Z

src/Containers/FileUpload/reducer.ts

+                    if(index===action.index)return false
+                    return true


Remove duplicate code @roggc

Also why not

return A == B

is A!==B, because I want all elements except the one for which A===B

ralph-dev

More code quality

ralph-dev · 2022-01-03T21:13:37Z

src/Containers/FileUpload/IsFailureIsSuccessPanel.tsx

 import { TextLayout } from '@Layouts';
 import { StyledIcon } from '@styled-icons/styled-icon';
-import { Container, Icon } from './StyledComponents';
+import { Container, Icon, IContainerProps } from './StyledComponents';


Does this component have its own story? Where is it?

no, I don't have a story for this styled component

Add a story for this component

src/Containers/FileUpload/FileUpload.tsx

ralph-dev · 2022-01-03T21:14:50Z

src/Containers/FileUpload/FileUpload.tsx

+                    doWithBase64StringFile(base64StringFile);
+                    setInformativePanelsState((prev) => ({
+                        ...prev,
+                        values: prev.values.map((value_) => {


value_ is an unclear name

it's because there exists already a value in scope. both value and value_ are of the same type (IValue)

So is one the previous value? And old value. Etc

ralph-dev · 2022-01-03T21:15:33Z

src/Containers/FileUpload/FileUpload.tsx

+                if(value_.name===value.name)return false
+                return true


Return A == B?
Or !(A == B)

ralph-dev · 2022-01-03T21:16:44Z

src/Containers/FileUpload/FileUpload.tsx

+                    ref={loadingContainerRef}
+                    overflow="hidden"
+                    width={
+                        value.isFailure.value || value.isSuccess.value


This code is duplicated in the next few lines why not make a function called

IsSuccessOrError?

ralph-dev · 2022-01-03T23:11:40Z

src/Containers/FileUpload/StyledComponents.ts

    positionTop?: number;
    flexGrow?: boolean;
+    /** transition duration in ms; applied on height and opacity properties; */
+    transitionDuration:number;


Transition of what duration??

ralph-dev · 2022-01-03T23:12:36Z

src/Containers/FileUpload/FileUpload.tsx

+    /** if true, failure message will appear even after success operation; its purpose is to test the appearance of the failure message during development */
+    isTestIsFailure?: boolean;
+    /** height and fade in-fade out animation duration in ms; default value: 200 */
+    animationDuration?: number;


Animation fo what duration?

ralph-dev · 2022-01-03T23:14:27Z

src/Containers/FileUpload/FileUpload.tsx

+            if (isFailureIsSuccessPanelProps) {
+                return (
+                    <IsFailureIsSuccessPanel
+                        {...isFailureIsSuccessPanelProps}


Does ...null work?

ralph-dev · 2022-01-03T23:14:42Z

src/Containers/FileUpload/FileUpload.tsx

+        [],
+    );
+
+    /** cancel uploading; terminate worker and remove informative panel */


Use JSDOC format

this is jsdoc

ralph-dev · 2022-01-09T01:17:01Z

src/Containers/FileUpload/FileUpload.tsx

+    /** if true, failure message will appear even after success operation; its purpose is to test the appearance of the failure message during development */
+    isTestIsFailure?: boolean;


This should not be needed.

ralph-dev · 2022-01-09T01:17:20Z

src/Containers/FileUpload/FileUpload.tsx

+    /**
+     * load array of informative panels (values) and send order to start workers
+     */


JSDOC missing Doc for Paramters / Return Type

ralph-dev · 2022-01-09T01:17:37Z

src/Containers/FileUpload/FileUpload.tsx

+    /**
+     * terminate worker and set state of informative panel to success or failure and
+     * send order to remove informative panel in the future. also do whatever user
+     * wants to do with the file read in case of success
+     */


Missing paramter / return

ralph-dev

Incorrect

roggc · 2022-01-12T17:56:29Z

@ralph-dev, what is incorrect?

ralph-dev · 2022-01-12T18:02:37Z

@ralph-dev, what is incorrect?

Create a NeW PR should be doing two work in same PR

roggc · 2022-01-12T18:21:57Z

@ralph-dev, what is incorrect?

Create a NeW PR should be doing two work in same PR

but it is related work

ralph-dev · 2022-01-12T18:30:37Z

@ralph-dev, what is incorrect?

Create a NeW PR should be doing two work in same PR

but it is related work

It is very difficult to review 15 files everytime when you are only changing a few. It's different work. One is modifying the existing, one is creating a new component entirely

ralph-dev · 2022-01-28T13:53:13Z

Let's close this PR if we don't need it @roggc

roggc added 3 commits January 2, 2022 10:46

refactor

0c77836

multiple file upload

ee3f360

jsdoc

e2e703c

roggc requested a review from ralph-dev January 2, 2022 12:11

roggc assigned ralph-dev Jan 2, 2022

ralph-dev suggested changes Jan 2, 2022

View reviewed changes

roggc added 2 commits January 3, 2022 15:38

parallel uploading without animation

d35e320

on cancel uploading

bd83307

ralph-dev suggested changes Jan 3, 2022

View reviewed changes

roggc added 2 commits January 3, 2022 21:21

refactor code

7e52054

add story isDisabled

9f647e2

roggc requested a review from ralph-dev January 3, 2022 21:03

ralph-dev suggested changes Jan 3, 2022

View reviewed changes

roggc added 3 commits January 3, 2022 23:35

some refactor - code quality

6bcf9c8

change the way start workers is processed

d26c743

code quality- variable names

c8c58a6

roggc requested a review from ralph-dev January 3, 2022 23:09

modify when workers are terminated

c1fab0d

ralph-dev reviewed Jan 8, 2022

View reviewed changes

roggc added 3 commits January 8, 2022 16:45

modify how workers are terminated when uploading is cancelled

6d36cb2

modify names of properties animationDuration and transitionDuration

76faad7

delete story about animation duration

a212b7e

ralph-dev reviewed Jan 9, 2022

View reviewed changes

fileuploadv2

3a71c97

roggc added 2 commits January 12, 2022 09:22

operationState

cfc3d09

panel

7dbb705

roggc requested a review from ralph-dev January 12, 2022 09:02

ralph-dev suggested changes Jan 12, 2022

View reviewed changes

stories for each component

d5c94e0

roggc requested a review from ralph-dev January 12, 2022 17:54

roggc added 2 commits January 12, 2022 23:39

delete fileuploadv2 changes

799da81

delete exports in index.ts

d8c1f05

		/** if true, failure message will appear even after success operation; its purpose is to test the appearance of the failure message during development */
		isTestIsFailure?: boolean;

Conversation

roggc commented Jan 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ralph-dev commented Jan 2, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

roggc commented Jan 2, 2022

Uh oh!

ralph-dev commented Jan 2, 2022

Uh oh!

ralph-dev left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

ralph-dev left a comment

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!

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!

ralph-dev left a comment

Choose a reason for hiding this comment

Uh oh!

roggc commented Jan 12, 2022

Uh oh!

ralph-dev commented Jan 12, 2022

roggc commented Jan 2, 2022 •

edited

Loading