Feat: made cards and developed event section (anitab-org#123) by Bucky25 · Pull Request #171 · anitab-org/anitab-org.github.io

Bucky25 · 2020-12-02T16:15:54Z

Description

I have implemented the Events card Component ,taking the data from events_and_highlights.js which will reflected on the events page of the web-app.

Fixes #123

Type of Change:

Delete irrelevant options.

Code

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:

Delete irrelevant options.

My PR follows the style guidelines of this project
I have performed a self-review of my own code or materials
I have attached link of deployed site.
Any dependent changes have been merged

Code/Quality Assurance Only

My changes generate no new warnings

nandini45 · 2020-12-07T15:35:50Z

@tichnas can you review this one?

tichnas · 2020-12-07T17:46:17Z

src/Components/Events/Cards/index.js

+import { withCard } from './../../../Decorators/Card';
+import Badge from './CardBadge';
+
+const EventCard = ({ props, isOver }) => {


isOver is not being used anywhere. Please remove it if not required

isOver is required

tichnas · 2020-12-07T17:47:51Z

src/Components/Events/Cards/index.js

+        <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) => (


Use index as a key to the child when using map (refer the warnings in the console)

good call with index as key.

tichnas · 2020-12-07T17:49:27Z

src/Components/Events/Cards/index.js

+        }}
+        onPress={() => {Linking.openURL(props.know_more.link)}}
+        >
+        {props.know_more.par}


I think it's just the words Know More > and not a full paragraph

know more has to substituted by para given in content i have asked it in issue

tichnas · 2020-12-07T17:50:13Z

src/Components/Events/index.js

+
+const events_highlight = getevents_highlights();
+
+class Events extends React.Component {


Use Functional components

please tell me what difference it will make as class component are better as for further usage of states

tichnas · 2020-12-07T17:51:59Z

src/Components/Events/Cards/index.js

+        <ScaledImage width={286}   source={props.highlights.source} />
+        <Text style={styles.title}>{props.title}</Text>
+        <Badge text={props.date} link={require('./../../../assets/events_and_highlights/calendar.png')}  />
+        <Badge text={props.location} link={require('./../../../assets/events_and_highlights/location.png')} />


I suggest using Icons instead of images (in Badge component)

for this we have to to install a dependency can i can do that like material or others

please move it to content js so each card have one source of truth regards the content. Thanks.

tichnas · 2020-12-07T17:57:13Z

src/Components/Events/Cards/index.js

+        color: '#103B81',
+        fontWeight: '400',
+        fontSize: 16,
+        marginTop: 12,


This should be 16 instead of 12

tichnas · 2020-12-07T18:01:21Z

src/Components/Events/Cards/index.js

+        borderRadius: 4,
+        overflow: 'hidden', 
+    },
+    cardContent: {


This is not used anywhere

tichnas · 2020-12-07T18:07:33Z

src/Components/Events/index.js

+              style={{
+                flexDirection: 'row',
+                flexWrap: 'wrap',
+                marginTop: 30,


Make this a power of 2, maybe 32

tichnas · 2020-12-07T18:08:06Z

src/Components/Events/index.js

+                        key={event_detail.title}
+                        props={event_detail}
+                        backgroundColor="#e7edfd"
+                        padding={12}


Make this a power of 2, maybe 16

tichnas

@Bucky25 Thanks for the PR. I've requested some changes which I think you should do before an admin review.

Bucky25 · 2020-12-07T20:43:43Z

@tichnas i have made all changes suggested by you and only one thing should i install any dependency to to make present icons but same type icons are in projects card so i did same as there and and about the functional component is is working fine
thanks for suggesting changes and pointing out some mistakes that i have made

Bucky25 · 2020-12-10T05:59:51Z

@nandini45 do i have to use another dependency to add icons of calendar , location or images that i have done is fine
as project cards also uses images to do that

nandini45 · 2020-12-12T17:48:21Z

@Bucky25 the project card one is fine

Bucky25 · 2020-12-12T18:02:29Z

@Bucky25 the project card one is fine

so do i have to change any thing else in this or its fine

tichnas · 2020-12-16T18:21:04Z

src/Components/Events/index.js

+
+const events_highlight = getevents_highlights();
+
+function Events () {


Use arrow function here for consistency as it's being used everywhere else.

there is no arrow function in other component index.js so i have also made the same .

Ok, leave it like this only then. Sorry for this.

tichnas

Delete package-lock.json file as it isn't required for yarn package manager (which we are using).

tichnas · 2020-12-16T18:40:17Z

@nandini45 To be honest, the cards look weird to me. Can you please take a look at the deployed website (https://bucky25.github.io/anitab-org.github.io/ > Events) to make sure that this was the design that was thought initially?
Also, if the implemented design is correct, can we please have another issue for redesigning the cards 😅

annabauza

Icons are too big and in wrong colors, apart from that I left several comments. Thanks

annabauza · 2020-12-22T19:31:10Z

src/Components/Events/Cards/index.js

+        <View style={{marginTop: 32}}>
+            {props.description.map((detail,index) => (
+                <Text
+                    style={{


please keep styles separately

annabauza · 2020-12-22T19:32:04Z

src/Components/Events/Cards/index.js

+        <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) => (


good call with index as key.

annabauza · 2020-12-22T19:34:32Z

src/Components/Events/Cards/index.js

+        <ScaledImage width={286}   source={props.highlights.source} />
+        <Text style={styles.title}>{props.title}</Text>
+        <Badge text={props.date} link={require('./../../../assets/events_and_highlights/calendar.png')}  />
+        <Badge text={props.location} link={require('./../../../assets/events_and_highlights/location.png')} />


please move it to content js so each card have one source of truth regards the content. Thanks.

annabauza · 2020-12-23T12:30:00Z

is this the same PR as #173 ?

Bucky25 · 2020-12-23T18:15:25Z

is this the same PR as #173 ?

no both are different issues and thanks for reviewing it

annabauza · 2020-12-26T11:57:14Z

Thanks @Bucky25 please change the label whenever it's ready to review :)

Bucky25 · 2021-01-23T03:50:10Z

@annabauza please review it once .

Vishal-raj-1 · 2021-01-29T17:39:11Z

@Bucky25 Please resolve conflicts and squash your commits !!

src/Components/Events/index.js

made cards and developed event section (anitab-org#123)

88b86e9

Bucky25 changed the title ~~Eeat: made cards and developed event section (anitab-org#123)~~ Feat: made cards and developed event section (anitab-org#123) Dec 2, 2020

nandini45 requested review from annabauza and removed request for annabauza December 4, 2020 11:48

tichnas reviewed Dec 7, 2020

View reviewed changes

src/Components/Events/Cards/index.js Outdated

borderRadius: 4,

overflow: 'hidden',

},

cardContent: {

Copy link

Contributor

tichnas Dec 7, 2020

Choose a reason for hiding this comment

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

This is not used anywhere

tichnas reviewed Dec 7, 2020

View reviewed changes

tichnas suggested changes Dec 7, 2020

View reviewed changes

tweaked styles

7ffeb44

tichnas reviewed Dec 16, 2020

View reviewed changes

tichnas suggested changes Dec 16, 2020

View reviewed changes

annabauza requested changes Dec 22, 2020

View reviewed changes

annabauza mentioned this pull request Dec 23, 2020

Feat: developed contribution section in project page #173

Open

5 tasks

annabauza added the Status: Changes Requested Changes are required to be done by the PR author. label Dec 23, 2020

made changes , decreased size of icons and added static text to content

a5f6955

made changes and moved text to content

6e65c2e

Bucky25 force-pushed the develop branch from e4b68f0 to 6e65c2e Compare December 28, 2020 14:06

Bucky25 requested a review from annabauza December 28, 2020 14:08

Merge branch 'develop' into develop

47a8026

annabauza reviewed Feb 7, 2021

View reviewed changes

src/Components/Events/index.js Outdated Show resolved Hide resolved

Apply suggestions from code review

ed7c1be

annabauza approved these changes Feb 7, 2021

View reviewed changes

annabauza merged commit 99d0c40 into anitab-org:develop Feb 7, 2021


		const events_highlight = getevents_highlights();

		class Events extends React.Component {


		const events_highlight = getevents_highlights();

		function Events () {

Conversation

Bucky25 commented Dec 2, 2020 • edited by nandini45 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of Change:

How Has This Been Tested?

Checklist:

Uh oh!

nandini45 commented Dec 7, 2020

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!

tichnas left a comment

Choose a reason for hiding this comment

Uh oh!

Bucky25 commented Dec 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Bucky25 commented Dec 10, 2020

Uh oh!

nandini45 commented Dec 12, 2020

Uh oh!

Bucky25 commented Dec 12, 2020

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!

tichnas left a comment

Choose a reason for hiding this comment

Uh oh!

tichnas commented Dec 16, 2020

Uh oh!

annabauza 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!

annabauza commented Dec 23, 2020

Uh oh!

Bucky25 commented Dec 23, 2020

Uh oh!

annabauza commented Dec 26, 2020

Uh oh!

Bucky25 commented Jan 23, 2021

Uh oh!

Bucky25 commented Dec 2, 2020 •

edited by nandini45

Loading

Bucky25 commented Dec 7, 2020 •

edited

Loading