Skip to content
This repository has been archived by the owner on Nov 2, 2021. It is now read-only.

Partial Bugfix #1013 : Missing state management #1022

Closed
wants to merge 5 commits into from
Closed

Partial Bugfix #1013 : Missing state management #1022

wants to merge 5 commits into from

Conversation

SensehacK
Copy link
Contributor

@SensehacK SensehacK commented Apr 12, 2020

Description of PR
This is a PR of displaying total tests checked and displaying it on the home page.
Sadly I was only able to get it working in my console window, don't know how to use state management in React.

Still this is my first time actually trying React and contributing to the project, Please suggest me how can I improve my code.

@KrishnaPravin has already implemented the PR bug fix. I'm just here to contribute what little I can to this website.

Type of PR

  • Bugfix
  • New feature

Relevant Issues
Fixes #1013

Checklist

  • Compiles and passes lint tests
  • Properly formatted
  • Tested on desktop
  • [] Tested on phone

Screenshots

Screen Shot 2020-04-12 at 7 27 44 PM

- Got the number working but can't figure how to update the state with div class.
- Printing the numbers in console.
Still don't know how to display the calculated data dynamically when function formatNumber is being called & returned '-' to replace it with my generated total numbers.
@vercel
Copy link

vercel bot commented Apr 12, 2020

This pull request is being automatically deployed with ZEIT Now (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://zeit.co/covid19india/covid19india-react/edym0zovo
✅ Preview: https://covid19india-react-git-fork-sensehack-master.covid19india.now.sh

totalTemp += Number(element);
});

setTotal(totalTemp);
Copy link
Contributor

Choose a reason for hiding this comment

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

remove the extra variable 'totalTemp'. You could use, js reduce function to setTotal. This way you would not be required to disable eslint and code will be more concise and readable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was my second attempt at React programming.

I knew I was missing state management of some sort.

Thanks for input. I’ll check & update the codebase.

Copy link
Contributor Author

@SensehacK SensehacK left a comment

Choose a reason for hiding this comment

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

@faisal3389 Please look into this, I have updated the code with JS reduce function.

Comment on lines 388 to 390
if (getMonth.length === 1) {
getMonth = '0' + getMonth;
}

Choose a reason for hiding this comment

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

This can be done in a single step using the padStart() in JS, https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/padStart

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, updated the code.

@jeremyphilemon
Copy link
Collaborator

@SensehacK Hey, thanks for the PR! I'm glad that you were able to get as far as you could, and that's great! I also feel this is a good start to learning react, I won't be able to merge this though, however, feel free to open up new PRs. If you have any questions in the process, we can maybe help you with the react part too!

Cheers :)

@SensehacK
Copy link
Contributor Author

@jeremyphilemon Thanks for the kind words, So far I have found React to be very quick to prototype stuff even with very less processing power.

Also I have raised a new PR #1390 . Please let me know your feedback.

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

Successfully merging this pull request may close these issues.

Total Number of tests
4 participants