-
-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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'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.
* adding useState at required page * removing unused index variable Co-authored-by: Abhi Jain <[email protected]> Co-authored-by: Eddie Jaoude <[email protected]>
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.
LGTM but would suggest one more reviewer that works in the project more often than I do
One edit you can consider: Defining CFP on the legend
Thoughts about possible follow up issues needed due to this update:
Is the mobile view too stacked with the legend now that there are 4 items? Possible need for a dropdown or some other type of filter on mobile?
This will require an update to documentation on the events directions likely as well
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.
You didn't update EventKey
with the new parameters here
Thanks @amandamartin-dev , comments inline below ...
Good point 👍
Yep, this is true 👍 hopefully it can come in another PR soon.
I think I have covered that, but if not let me know |
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.
The changes look good.
Just waiting on the cfpOpen filter check in pages/events.js
OMG I missed that 🤦♂️ thank you Dan 🔥 I will do that now |
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.
Looks good
Fixes Issue
fixes #2528
fixes #2677