-
Notifications
You must be signed in to change notification settings - Fork 190
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
Feat: developed contribution section in project page #173
base: develop
Are you sure you want to change the base?
Conversation
@gaurivn can u test this PR?? |
src/Components/Events/Cards/index.js
Outdated
import { withCard } from './../../../Decorators/Card'; | ||
import Badge from './CardBadge'; | ||
|
||
const EventCard = ({ props, isOver }) => { |
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.
Please remove isOver
as it's not used anywhere
src/Components/Events/Cards/index.js
Outdated
<Badge text={props.location} link={require('./../../../assets/events_and_highlights/location.png')} /> | ||
<Badge text={props.timings} link={require('./../../../assets/events_and_highlights/time.png')} /> | ||
<View style={{marginTop: 32}}> | ||
{props.description.map((detail,index) => ( |
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.
use index
as key in the child when using map
src/Components/Events/index.js
Outdated
marginTop: 30, | ||
}} | ||
> | ||
{events_highlight.sections[1].events.map((event_detail,index) => ( |
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.
Please remove index
as it's not being used anywhere
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.
@Bucky25 I've requested some changes which I think should be done.
Also, please try to avoid inline styles and class components for consistency in the code and put all margins/paddings as a power of 2.
Thanks :)
} | ||
]; | ||
|
||
function Contribution () { |
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.
Use arrow function here for consistency as it's being used everywhere else.
src/Components/Projects/index.js
Outdated
@@ -5,6 +5,9 @@ import { View } from 'react-native'; | |||
import ImageTextSection from './../ImageTextSection'; | |||
import { getProjects } from './../../content/projects_content'; | |||
import ProjectCard from './ProjectCard'; | |||
import Calander from './Contribution'; |
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.
Change Calander
to Calendar
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.
Delete package-lock.json file as it isn't required for yarn package manager (which we are using).
Also, instead of using var
, use const
or let
.
import { View, Text, StyleSheet } from 'react-native'; | ||
import ContributionBox from './Box'; | ||
|
||
class ContributionRow extends React.Component { |
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.
Change class component to functional component (preferably arrow function).
I know both are same in terms of performance, but functional components are more concise and modern (and are used everywhere else). They don't have any disadvantages but may get optimized in future. Please refer https://www.twilio.com/blog/react-choose-functional-components for more information.
Do let me know if you face any trouble with functional components.
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.
ok i will use useEffect there and it will be done
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.
@Bucky25 Are you saying about componentDidMount
? If yes, there is a React hook called useEffect
which you can use (refer to the link shared above)
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.
Icons are wrong. @Vuyanzi @Sonal240 @simranaggarwal1999 can any of you provide proper icons please. Thanks a lot.
src/Components/Content/index.js
Outdated
@@ -12,7 +12,7 @@ function Content({ selected, titles }) { | |||
return <AboutUs />; | |||
} else if (selected === 3) { | |||
return <Projects />; | |||
} | |||
} |
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.
} | |
} |
var hue = 0; | ||
switch (true) { | ||
|
||
case (props < 2): |
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.
can't we just use interpolation here?
import ContributionRow from './ContributionRow'; | ||
import ContributionBox from './Box'; | ||
|
||
const data = [ |
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.
move this to content folder please.
} | ||
<View style={styles.description}> | ||
<Text style={styles.text}>Less</Text> | ||
<ContributionBox props={1} /> |
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.
I think it will be good to rename it from props. The name is misleading.
is this the same PR as #171 ? |
2acb6a8
to
650b8a8
Compare
@Bucky25 Please squash your commits into one commit !! |
@Vishal-raj-1 we squash when merge. No need to squash. |
@annabauza that's great !! |
@annabauza thanks |
@Bucky25 any pending work needs to be done in the pr?? |
@nandini45 i think its done i have done all the changes required ? |
@Bucky25 yes i will ping Anna again for her review |
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.
100
|
||
const ContributionBox = ({ commitCount }) => { | ||
var hue = 0; | ||
switch (true) { |
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.
This is dodgy i think simple if else would do here
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.
done @annabauza please check it
@Bucky25 It contains conflicting files. Resolve it !! |
as files already present
@annabauza please remove these files: |
@annabauza can you please provide your feedback ? |
@Bucky25 any updates about solving the conflicts? |
i already mentioned that these files are existing in the repo and are different from pr because new one are that png files . |
@annabauza can you review the PR please? |
@isabelcosta Can you review the PR ? |
I can try but my frontend skills are a bit rusty, could you ask Zulip to see if anyone wants to help. @nandini45 |
@Bucky25 are you still interested in finishing this issue? |
Description
Used GitHub API to get commit data and made a contribution heat map for last 30 days
patch #80
Fixes #80
Type of Change:
Delete irrelevant options.
Code/Quality Assurance Only
Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality pre-approved by mentors)
How Has This Been Tested?
I have tested the implementation and responsiveness.
here is the deployed link.
https://bucky25.github.io/anitab-org.github.io/
Checklist:
Code/Quality Assurance Only