Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

improve our code #132

Open
atherdon opened this issue Mar 24, 2019 · 27 comments
Open

improve our code #132

atherdon opened this issue Mar 24, 2019 · 27 comments

Comments

@atherdon
Copy link
Member

in order to see warnings that we have (we're using module standard, but it's just a wrapper around ESLint) - you need to run yarn code-fix and you'll see warnings that we have
This can be your second task @ronniebhatt

@ronniebhatt
Copy link

what problem i should solve

@atherdon
Copy link
Member Author

you need to clean up warnings.
so when you'll run that command - you'll see default ESLint warnings(like - you include A but didn't use it).
And your goal is to fix that warnings

@ronniebhatt
Copy link

i created new pull request, can you check it

@atherdon
Copy link
Member Author

#136 (review)

@ronniebhatt
Copy link

sir ,without installing that dependency , i couldn't run yarn code-fix

@ronniebhatt
Copy link

sir, can you tell me how to run yarn code-fix without installing that dependency

@atherdon
Copy link
Member Author

atherdon commented Mar 25, 2019 via email

@ronniebhatt
Copy link

can you give me some other task, because i can't run this command on my editor(npm run code-fix)

@atherdon
Copy link
Member Author

ok, let's jump here: Food-Static-Data/sd#61

@atherdon
Copy link
Member Author

actually it's strange. I assume it's a mis-understanding from your side what you need to do.
Because in your last pull request you install our module, so I assume you're using npm from your terminal and run npm install @groceristar/pdf-export and for displaying this warnings you need to run a command from a terminal too.

another point. did you install packages when you grab code locally? i mean in order to run a project locally- you need to save code on your computer, open a terminal at project folder and run 'npm install'.

you can check this video: https://www.youtube.com/watch?v=Z1LDNELRqYA

btw, this is a list of warnings that need to be fixed

derList.test.js:7:8: 'RenderList2' is defined but never used.
  /home/art/Documents/C/react/GS/PDF/pdf-export-component/src/components/Ren
derList.test.js:8:8: 'RenderList3' is defined but never used.
  /home/art/Documents/C/react/GS/PDF/pdf-export-component/src/components/Ren
derList.test.js:9:8: 'RenderList4' is defined but never used.
  /home/art/Documents/C/react/GS/PDF/pdf-export-component/src/components/Ren
derList.test.js:19:1: 'describe' is not defined.
  /home/art/Documents/C/react/GS/PDF/pdf-export-component/src/components/Ren

derLists3/List.js:4:22: 'StyleSheet' is defined but never used.
  /home/art/Documents/C/react/GS/PDF/pdf-export-component/src/components/Ren
derLists4/RenderLists4.js:3:22: 'StyleSheet' is defined but never used.

@atherdon
Copy link
Member Author

I think maybe this warnings was resolved, because we rename our component.s But i'm sure that our code is not ideal. still not ideal :)

@atherdon
Copy link
Member Author

i printed new warnings for you

D:\code\pdf-export-component\src\components\DocumentLayouts.js:37:5: Duplicate case label.
  D:\code\pdf-export-component\src\components\DocumentLayouts.js:48:30: 'styles' is not defined.
  D:\code\pdf-export-component\src\components\DocumentLayouts.js:49:22: 'styles' is not defined.
  D:\code\pdf-export-component\src\components\DocumentLayouts.js:50:24: 'styles' is not defined.
  D:\code\pdf-export-component\src\components\DocumentLayouts.test.js:1:8: 'React' is defined but never used.
  D:\code\pdf-export-component\src\components\DocumentLayouts.test.js:2:8: 'ReactDOM' is defined but never used.
  D:\code\pdf-export-component\src\components\DocumentLayouts.test.js:3:8: 'Enzyme' is defined but never used.
  D:\code\pdf-export-component\src\components\DocumentLayouts.test.js:3:29: 'shallow' is defined but never used.
  D:\code\pdf-export-component\src\components\DocumentLayouts.test.js:3:38: 'mount' is defined but never used.
  D:\code\pdf-export-component\src\components\DocumentLayouts.test.js:6:8: 'RenderList1' is defined but never used.
  D:\code\pdf-export-component\src\components\DocumentLayouts.test.js:7:8: 'RenderList2' is defined but never used.
  D:\code\pdf-export-component\src\components\DocumentLayouts.test.js:8:8: 'RenderList3' is defined but never used.
  D:\code\pdf-export-component\src\components\DocumentLayouts.test.js:9:8: 'RenderList4' is defined but never used.

we can discuss each case and decide what you'll need to do

@ronniebhatt
Copy link

ok i will check

@ronniebhatt
Copy link

so first i have to run npm install then npm run code-fix, right

@ronniebhatt
Copy link

@ronniebhatt
Copy link

please check

@atherdon
Copy link
Member Author

atherdon commented Mar 30, 2019 via email

@ronniebhatt
Copy link

i think that the variable which are declared and are not used they should be removed first

@atherdon
Copy link
Member Author

atherdon commented Apr 1, 2019

ok, put here a list of files, that require this change.


What else we'll do next(as previous step will be done soon)

@atherdon
Copy link
Member Author

atherdon commented Apr 1, 2019

this task you should complete.

@ronniebhatt
Copy link

So i should run npm code-fix and solve the error ,right?

@atherdon
Copy link
Member Author

atherdon commented Apr 1, 2019 via email

@ronniebhatt
Copy link

sorry for that, now i rechecked the error i am only getting now( Parsing error: Unexpected token =), but can't see any problem in that

@ronniebhatt
Copy link

Sorry, Sir i won't be able to solve this task

@ronniebhatt
Copy link

Sir, actually I want to increase my skills on react, so if it is possible please give me some react task so that I will learn something new on react , and increase my knowledge

@atherdon
Copy link
Member Author

atherdon commented Apr 2, 2019

you pushing me same idea again and again. looks like you know what you want.
So I can go hard on you :)
Again, if you think that this both tasks that you've done are "not React" - maybe you don't know something.

@atherdon
Copy link
Member Author

atherdon commented Apr 2, 2019

you can grab it: #145

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants