Skip to content
This repository has been archived by the owner on Aug 7, 2024. It is now read-only.

I have added filter options to see events virtual/in person #2605

Merged
merged 3 commits into from
Jan 5, 2023
Merged

I have added filter options to see events virtual/in person #2605

merged 3 commits into from
Jan 5, 2023

Conversation

abhijain2003
Copy link
Contributor

@abhijain2003 abhijain2003 commented Jan 3, 2023

Fixes Issue

#2528

Changes proposed

-> initialize state variable in events.js file with toggle values "virtual"/"person"
-> passing setState method in eventKey.js file as a props so I can toggle values on button clicks
-> implementing filter method before map in events.js file based on value of state variable
-> added a scale-105 class on hover in EventKey.js file

Screenshots

Events.the.community.members.are.going.to.-.Google.Chrome.2023-01-03.10-05-34.mp4

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

It's great having you contribute to this project

Welcome to the community 🤓

If you would like to continue contributing to open source and would like to do it with an awesome inclusive community, you should join our Discord chat and our GitHub Organisation - we help and encourage each other to contribute to open source little and often 🤓 . Any questions let us know.

@github-actions github-actions bot added the large Pull request with more than 30 changed lines label Jan 3, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Jan 3, 2023

Welcome @abhijain2003! Thank you so much for your first pull request!

))}
)
)
})}
Copy link
Member

Choose a reason for hiding this comment

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

These changes seem to be the same but with a console log?

pages/events.js Outdated Show resolved Hide resolved
pages/events.js Outdated Show resolved Hide resolved
pages/events.js Outdated Show resolved Hide resolved
@abhijain2003
Copy link
Contributor Author

can you give me one more chance I'll resolve all conversation

Copy link
Member

@eddiejaoude eddiejaoude left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution 👍

  • I think the package-lock file does not need to be changed
  • I left some inline comments

@eddiejaoude
Copy link
Member

can you give me one more chance I'll resolve all conversation

of course, we are here to help, any questions let us know 👍

@abhijain2003
Copy link
Contributor Author

can I add a small button to take back to previous stage and show all events

@abhijain2003
Copy link
Contributor Author

I have refactor the code
remove useless function in EventKey.js and calling onChange Directly
renamed onChange to onToggleEventType
renamed child inside filter method from itm to event
replaced _ with index
displaying all events by default

didn't made any changes in package-lock previously so didn't toched it now as well
I don't think it can be done without useState variable so, I did not changed it

Can I commit it or would you like to give me more advices

@eddiejaoude
Copy link
Member

Great! Always commit otherwise we can't see the changes - GitHub will append them to this PR for review

@ChinmayMhatre
Copy link
Member

I have refactor the code remove useless function in EventKey.js and calling onChange Directly renamed onChange to onToggleEventType renamed child inside filter method from itm to event replaced _ with index displaying all events by default

didn't made any changes in package-lock previously so didn't toched it now as well I don't think it can be done without useState variable so, I did not changed it

Can I commit it or would you like to give me more advices

Hey @abhijain2003 , I think what Eddie means. By the useState is that the import isn't used in UserEvent.js so you could remove it. And Also that the console.log should be removed from everywhere after you are done debugging.

@abhijain2003
Copy link
Contributor Author

ok
I just solved all issues hope this is ready to merge now

@ChinmayMhatre
Copy link
Member

I think we should add a border to the selected event type. What do you think @abhijain2003 , @eddiejaoude ?

@ChinmayMhatre
Copy link
Member

ok I just solved all issues hope this is ready to merge now

Hey , I think eddie also had suggested to remove the package-lock.json from the pr.

@abhijain2003
Copy link
Contributor Author

I think we should add a border to the selected event type. What do you think @abhijain2003 , @eddiejaoude ?

I added a kind of scale property on hover on selected button

@eddiejaoude
Copy link
Member

I think we should add a border to the selected event type

I think this can come in another PR

@abhijain2003
Copy link
Contributor Author

so will you merge it now

@eddiejaoude
Copy link
Member

so will you merge it now

@abhijain2003 please review remaining inline comments

@abhijain2003
Copy link
Contributor Author

ok eddie

@abhijain2003
Copy link
Contributor Author

eddie as you told me I have removed package-lock file and made a new commit

hope there are no further checks are remaining

@eddiejaoude
Copy link
Member

eddie as you told me I have removed package-lock file and made a new commit

Please do not remove this file, it is important, I asked to remove your changes to the file. You can use git checkout main --- package-lock.json and this should return the file to the state before your changes.

Also please address the remaining 2 inline comments that are not in a resolved state yet (these are the ones that are still expanded on this page (the others are collapsed as they are resolved)

@abhijain2003
Copy link
Contributor Author

hey Eddie, I think I messed up the contribution, but I resolved all conversations now,

Thanks for Helping

@eddiejaoude
Copy link
Member

@abhijain2003 you marked this as resolved without a comment or changing the lines of code it is referring to, this makes it difficult to review. Do you need some help with this? I have marked it back to unresolved.

Screenshot 2023-01-04 at 09 53 13

@abhijain2003
Copy link
Contributor Author

abhijain2003 commented Jan 4, 2023

not at all eddie this button was by mistaken pressed I am sorry for that,

and I made changes in code, removed unused imports and this issue is also resolved

and even the changes you can see was becuase initially I write my code in wrong file, then I changed before my first pr

that's why github is showing those changes

@eddiejaoude
Copy link
Member

and even the changes you can see was becuase initially I write my code in wrong file

can you remove any change you don't want. Please see my inline comment https://github.com/EddieHubCommunity/LinkFree/pull/2605/files#r1060438032

@abhijain2003
Copy link
Contributor Author

done eddie , i commited to my branch all changes

hope now it is resolved

pages/events.js Outdated
@@ -20,6 +21,8 @@ export async function getServerSideProps(context) {
}

export default function Events({ events }) {

const [eventType, seteventType] = useState('');
Copy link
Member

Choose a reason for hiding this comment

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

useState will need an import

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thank eddie, for letting me know I just added it

pages/events.js Outdated
<EventCard event={event} username={event.username} key={index} />
))}
{events
.filter((event, index) => {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think index is used here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes you are right, I have removed it

@eddiejaoude eddiejaoude changed the base branch from main to event-filter January 5, 2023 07:55
@github-actions github-actions bot added small Pull request with less than 10 changed lines and removed large Pull request with more than 30 changed lines labels Jan 5, 2023
@eddiejaoude
Copy link
Member

Thank you 👍 I will resolve conflicts

@abhijain2003
Copy link
Contributor Author

thanks to you, you really helped me in my initial stage of open source looking forward to contribute more

@eddiejaoude eddiejaoude merged commit 2bef633 into EddieHubCommunity:event-filter Jan 5, 2023
@eddiejaoude
Copy link
Member

Thank you for your hard work @abhijain2003 💪

eddiejaoude added a commit that referenced this pull request Jan 7, 2023
* I have added filter options to see events virtual/in person

* resolving issues renamed descriptive variables and removed unused variables

* Update events.js

* removing package-lock file

* removed unused import

* removing unused changes

* feat: events filter (#2605)

* adding useState at required page

* removing unused index variable

Co-authored-by: Abhi Jain <[email protected]>
Co-authored-by: Eddie Jaoude <[email protected]>

* fix: remove unrequired changes

* feat: filter events by cfp #2677

* fix: user events list

* fix: cfp filter for events

Co-authored-by: abhi jain <[email protected]>
Co-authored-by: Abhi jain <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
small Pull request with less than 10 changed lines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants