pymc-devs / pymcon_web_series_website

http://www.pymcon.com
5 stars 11 forks source link

Add Event Cards #60

Closed purna135 closed 1 year ago

netlify[bot] commented 1 year ago

Deploy Preview for pymcon ready!

Name Link
Latest commit 31bb8e3743876e6acd80ab2dbc0e45b29cde14b3
Latest deploy log https://app.netlify.com/sites/pymcon/deploys/63c6b2b79e42660008b713fd
Deploy Preview https://deploy-preview-60--pymcon.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

canyon289 commented 1 year ago

This is amazing, ill do a full review in next 8 hours. One question, can the colors for upcoming and past events be made the same?

image image
purna135 commented 1 year ago

Sure, which colour do you prefer? 😀

canyon289 commented 1 year ago

Upcoming seems nice

purna135 commented 1 year ago

Done, additionally, I updated the button's hover colour from red to the same bluish color; let me know which one you think looks best.

canyon289 commented 1 year ago

This is great! My vote is to merge this but after disabling autodeploy in CI. That way we can then update the website with the latest event details and such

OriolAbril commented 1 year ago

This is great! My vote is to merge this but after disabling autodeploy in CI. That way we can then update the website with the latest event details and such

You could change the on push main trigger for workflow_dispatch to be able to trigger it manually but still have everything be done with github actions and still build but not deploy in previews to catch potential errors. Example: https://github.com/pymc-devs/pymc-data-umbrella/blob/main/.github/workflows/tx-pull.yml#L6

purna135 commented 1 year ago

You could change the on push main trigger for workflow_dispatch to be able to trigger it manually but still have everything be done with github actions and still build but not deploy in previews to catch potential errors. Example: https://github.com/pymc-devs/pymc-data-umbrella/blob/main/.github/workflows/tx-pull.yml#L6

This is really great.

merge this but after disabling autodeploy in CI.

I don't think we need to disable it; I'll remove the demo content in my next commit and add a few conditions for no events.

OriolAbril commented 1 year ago

I still see the goals before the events though, we should update before merging so we don't forget.

I like the card layout in the homepage, but I am not sure about the interaction. There are issues I am not sure are issues and others I am not sure how to go about them so please answer how you feel about them so we can get different points of view. cc @canyon289 @mjhajharia @purna135

Regarding the event page, I feel past events are a bit too hidden. Would it be possible to keep the right side sidebar but have the elements act more as a table of contents thing? e.g. all events are there, sorted chronologically with past ones at the bottom, and there is a "past events" heading separating upcoming from past events which is where the past events text on the right sidebar links to.

purna135 commented 1 year ago

Regarding the event page, I feel past events are a bit too hidden. Would it be possible to keep the right side sidebar but have the elements act more as a table of contents thing? e.g. all events are there, sorted chronologically with past ones at the bottom, and there is a "past events" heading separating upcoming from past events which is where the past events text on the right sidebar links to.

Could you please check now that I have added a new button for all of the events?

Let me know if you want to fix the position of the right side buttons on scroll.

purna135 commented 1 year ago
  • My main issue is with the "scroll/move left animation". I like to take my time doing things but I am not super slow either. I am unable to read the title plus description of the latest event before it disappears from view, to me this removes attention from the event appearing first when I think we'd want that to be the most eye catching.

The "scroll/move left animation" will be enabled only when there are more than three featured events; will there be more than three featured events in the future? If so, I will fix this issue.

canyon289 commented 1 year ago

Thanks @OriolAbril and @purna135. I trust between you both these details can get figured out. I left some comments below

My main issue is with the "scroll/move left animation". I like to take my time doing things but I am not super slow either. I am unable to read the title plus description of the latest event before it disappears from view, to me this removes attention from the event appearing first when I think we'd want that to be the most eye catching.

We shouldn't have more than 3 at a time. Either way its a setting in the JS library to disable it

I don't understand the order event cards appear in the homepage. I think that should be chronological but it doesn't look like it. I believe this this is coming from the event order setting in the toml which is what I asked. I think this is demo content right now If so we can modify it right @purna135?

Whole card click on front page This would be great actually. If possible can we do this?

I don't think we need to disable it; I'll remove the demo content in my next commit and add a few conditions for no events.

You've already done an incredible job on this website building the framework. If you'd like I'd be happy to ask the event mentors to add their events themselves. Let me know

purna135 commented 1 year ago

I don't understand the order event cards appear in the homepage. I think that should be chronological but it doesn't look like it.

I believe this this is coming from the event order setting in the toml which is what I asked. I think this is demo content right now If so we can modify it right @purna135?

Yes, we are sorting the events in ascending order based on the "eventOrder" property.

purna135 commented 1 year ago

I still see the goals before the events though, we should update before merging so we don't forget.

✅ Done

  • My main issue is with the "scroll/move left animation".

✅ Done, because we only display three events as featured events, the scroll animation will not appear, and even if we increase the number of featured events in the future, I have fixed the scroll animations.

  • Even if I hover and activate the animation for the card background, the scrolling out of view does not stop.

✅ Fixed, The animation will now be paused on mouse hover, mouse click, and keyboard focus.

  • I don't understand the order event cards appear in the homepage. I think that should be chronological but it doesn't look like it.

✅ We have a "eventOrder" property that allows us to control the order in which events are displayed.

  • I want to click on the card so badly! But the link is only the learn more button.

✅ We can now click on the entire card, which will take us to an external event post.

  • render the author as a link to their website of their preferred social link.

✅ Done, now we can click on the Author image/name.

I feel past events are a bit too hidden

✅ We now have three buttons: all events, upcoming events, and past events (all events is the default), also made the buttons sticky on scroll.

👉NOTE: I've added some conditional statements that keep the current layout unchanged when there are no events to display, so we can merge this PR without breaking the autodeploy CI.

👉LAST STEP: Once we've verified that everything is in order, we can remove the dummy data from event.toml and merge this PR to begin filling out the event details in separate PRs. (Please let me know when we are ready to merge this PR, and I will push my final commit, which deletes the dummy event data.)

Also, I used some demo headings and descriptions (for example, PyMCon Events and its description on the event page), so please suggest some better headlines.

@canyon289 @mjhajharia @OriolAbril Please take a look and make any suggestions for improvement.

Thank you 😊

canyon289 commented 1 year ago

No action needed on this comment. i checked the website for accessibility. Were good here. Thank you Purna for handling this

image
cluhmann commented 1 year ago

I am late to this, but I just wanted to quick mention that the only thing I found a bit odd is the the "expand on hover" function makes it very difficult to click on the "Learn More" button because that button moves when the box expands.

purna135 commented 1 year ago

Agreed, I removed the read more button from the hover effect, but I think it's better to remove the hover effect altogether, what do you think? Could you please take a look now?

purna135 commented 1 year ago

I removed the hover effect because there is a read more button to get more details.

cluhmann commented 1 year ago

Yeah, that works. I think the cards look great. In terms of keeping a uniform size across events, I think we can edit the short bit of text for future events so that it matches the existing events and helps to maintain equivalent sizing.

purna135 commented 1 year ago

I think we can edit the short bit of text for future events so that it matches the existing events

There won't be a problem with the card's size because the extra information will be hidden and just the text that fits the space will be displayed.

cluhmann commented 1 year ago

Even better!

purna135 commented 1 year ago

Is there anything else we need to do with this PR?

canyon289 commented 1 year ago

For now I dont think so. Thank you I just need to get event cards up. i can do so this week

purna135 commented 1 year ago

Thank you for your confirmation; let me to remove the dummy event data so that we can merge this PR.

canyon289 commented 1 year ago

I created another PR against your branch to include an actual event card as well.

Here it is. If it looks good to you mind merging it so we can do a final check?

https://github.com/purna135/pymcon_2022_website/pull/1/

canyon289 commented 1 year ago

@purna135 thanks much for this PR I'll detail specifically what I think the next sequence should be

  1. Merge this PR from me into this branch so we can inspect the final result https://github.com/purna135/pymcon_2022_website/pull/1
    • Also then once we merge we'll have an actual event on the page
  2. Each event mentor can follow up with their own PR to add their event

WDYT?

canyon289 commented 1 year ago

Thanks Purna One last question, do you know where we can replace the default speaker image with one thats more gender and skin color neutral? Dont feel obligated to do this yourself, youve already done great work on this PR. just want your guidance on how we can go about this for DEi

image
purna135 commented 1 year ago

do you know where we can replace the default speaker image with one thats more gender and skin color neutral?

pymcon_web_series_website/static/images/speakers

here we have a default image. I recently updated the default image, could you please verify if it is suitable for use?

canyon289 commented 1 year ago

do you know where we can replace the default speaker image with one thats more gender and skin color neutral?

pymcon_web_series_website/static/images/speakers

here we have a default image. I recently updated the default image, could you please verify if it is suitable for use?

Yes the image is perfect,

For the featured image, allowing for them to show up if there's at least 1 would be fantastic. Most of the time we'll only have 1 to 3 as is.

cluhmann commented 1 year ago

This is looking good to me. I suspect that we'll be able to figure out things to tweak once we start adding in real events. Do we want the first event to be cleaned up before merging?

canyon289 commented 1 year ago

This is looking good to me. I suspect that we'll be able to figure out things to tweak once we start adding in real events. Do we want the first event to be cleaned up before merging?

Id say we merge right after we get the single featured event stuff working and clean up second so we can take this off purnas plate, as hes already done a tremendous amount.

cluhmann commented 1 year ago

Yup, I think that makes sense.

cluhmann commented 1 year ago

How is this PR looking to you @canyon289 ? Anything left to do before merging or can we merge it and then start adding the real info for specific events?

canyon289 commented 1 year ago

We want featured events to show up even if theres less than three but we can do that on a follow up. Merging this as is

canyon289 commented 1 year ago

Thank you very much @purna135 for getting this together for us