Skip to content
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

Upcoming events page #338

Open
wants to merge 13 commits into
base: migration
Choose a base branch
from
Open

Upcoming events page #338

wants to merge 13 commits into from

Conversation

prishat2005
Copy link

@prishat2005 prishat2005 commented Nov 21, 2024

Status:

Description

I edited the upcoming events page to make the modals structured properly and of a proper size. I made the dates and day of week formatted properly.

Screenshots

image

@prishat2005 prishat2005 requested a review from inolasv November 21, 2024 01:49
Copy link
Collaborator

@inolasv inolasv left a comment

Choose a reason for hiding this comment

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

Great start! I made a few comments (most are trivial) on the code itself, but one thing i wanted to bring up was that maybe we can anchor some parts of the events to sides of the box to keep consistent padding. I know this might be a lot so let me know if you have any questions about anything!

what we currently have
Come to our office to chat,

what we want
Screenshot 2024-11-20 at 9 13 50 PM

src/sections/UpcomingEventsSection.js Outdated Show resolved Hide resolved
src/sections/UpcomingEventsSection.js Outdated Show resolved Hide resolved
.eslintrc.json Outdated Show resolved Hide resolved
src/styles/components/EventDescriptionModal.module.css Outdated Show resolved Hide resolved
src/styles/components/EventDescriptionModal.module.css Outdated Show resolved Hide resolved
src/styles/components/PastEventsButton.module.css Outdated Show resolved Hide resolved
src/styles/components/UpcomingEvent.module.css Outdated Show resolved Hide resolved
src/styles/pages/Home.module.css Outdated Show resolved Hide resolved
yarn.lock Outdated Show resolved Hide resolved
src/styles/components/UpcomingEvent.module.css Outdated Show resolved Hide resolved
@prishat2005 prishat2005 requested a review from inolasv November 24, 2024 06:11
Copy link
Collaborator

@inolasv inolasv left a comment

Choose a reason for hiding this comment

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

(@BeckyBlake tagging you here too)
Looks good, a couple things to add:

  • can you make sure that when we click on the red dot we close out the modal as well (to keep in line with what we've been doing so far)
  • could the lines here be a little less padded:
Screenshot 2025-02-13 at 4 01 45 PM

src/styles/components/EventDescriptionModal.module.css Outdated Show resolved Hide resolved
@@ -1,11 +1,16 @@
.container {
color: black;
display: flex;
gap: 0.5rem;
/* gap: 0.5rem; */
Copy link
Collaborator

Choose a reason for hiding this comment

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

you can removew the commented code

}

.hiddenButton {
border: none;
background-color: transparent;
/* added */
width: 100%;
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove added comments before committing

topbarColor = '#65C7CC',
buttonColor = '#CBEDFF',
topbarColor = 'var(--wcs-blue)',
buttonColor = '#D1EEEF',
Copy link
Collaborator

Choose a reason for hiding this comment

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

this color should be var(light-blue)

@@ -125,16 +121,19 @@
.eventContainer {
padding: 3.75rem;
}

.pastEventsButtonText {
font-size: 20px;
Copy link
Collaborator

Choose a reason for hiding this comment

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

just as a note, we would ideally not want to use px here, and instead use either built in h1/h2 etc or use a more responsive sizing. It is fine for right now, but we'll be working on adding responsiveness later, so a good think to keep note of.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants