-
Notifications
You must be signed in to change notification settings - Fork 1
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
Add react-svg-piechart #24
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks nice - I would love to see any tooltips over those charts when hovering. Enlarge 'effect' rocks! :)
src/topic/components/Participants.js
Outdated
import Colors from '../../styles/Colors'; | ||
|
||
const Participants = props => { | ||
const VOTES_NEEDED = 40; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as this is a render method, you are re-declaring const meant in each rendering. What do you think to move out the VOTES_NEEDED and Chart Data out of this function?
src/topic/components/Participants.js
Outdated
import PieChart from 'react-svg-piechart'; | ||
import Colors from '../../styles/Colors'; | ||
|
||
const Participants = props => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A general rule of thumb is - if you are using any object more than once, destruct it! (you may want to take a look at fb's react's example there
In Participants
we have it twice, so we can improve it by declaring:
const Participants = ({likes}) => { ... }
src/topic/components/Participants.js
Outdated
return (<StyledParticipants> | ||
<PieChart | ||
data={chartData} | ||
// If you need expand on hover (or touch) effect |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[opinion/minor] I believe that props are more like self-explandatory - thus no additional comments are necessary
* Add react-svg-piechart * Update after CR
* chore: update Trainer component and add tests * chore: add TrainersList tests * Disable lint for redux devtools extension * chore: update argument naming to be more meaningful * Add react-svg-piechart (#24) * Add react-svg-piechart * Update after CR
No description provided.