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

changed schedule page for VH 2024 #261

Merged
merged 9 commits into from
May 24, 2024
Merged

changed schedule page for VH 2024 #261

merged 9 commits into from
May 24, 2024

Conversation

alicetran1221
Copy link
Collaborator

No description provided.

@alanchangxyz
Copy link
Collaborator

good start on this 🥳 there's a lot to go through here but i'll start off with some opening comments:

  • generally images can be named almost whatever sounds relevant, but we should keep the formatting scheme consistent - let's stick with the names being hyphenated (instead of spaces) and lowercase (e.g. schedule-header.png instead of Schedule Header.png)
  • if we're going to be using colors a lot (like #ee99a0 or #322660) we can use the color variables defined in globals/vh-styles.scss; for scss, we import these and use them like backgroundColor: $purple so that we don't have to hardcode colors where we don't need to. feel free to change the old colors out in globals/vh-styles.scss if they've been replaced in the style kit already (cc @nk-soon )
  • while i think the current activity-pill.png is fine visual wise, we could probably achieve the same thing with just jsx/css in a reusable react component; otherwise we'd just have to decorate each instance of the activity tag with the appropriate arguments (aria labels, alt texts, etc.)
  • can we try to get the workshop times all on one line (and perhaps on the same one as the event titles even when there are activity tags for the events)? + the characters look kind of squished together, we can probably tweak the letter-spacing css property to make them a little more spaced out
image
  • slightly ocd but the top part of the countdown is cut off when i'm looking at it from my mac:
image
  • looks like we have a merge conflict here, gonna need to resolve this by getting the latest version of main branch and then merging it into this one (choosing the part(s) of the code you want to keep along the way)

@alicetran1221
Copy link
Collaborator Author

alicetran1221 commented May 22, 2024

just some concerns i had:

  • the countdown timer is cut off at the top but im not sure how to fix that and maybe im overlooking a part of the code that adjusts the container size for it
  • schedule info is still in progress since im waiting for corporate for accurate info
  • i used "inspect" to look at the page in iphone dimensions and the page seems very off center

other than that, i fixed the above stuff

@alanchangxyz
Copy link
Collaborator

i think we're still quite a bit off from having it be done, besides the stuff mentioned in your comment:

  • Schedule Header.png still needs to be hyphenated
  • there's a lot of extra images in there now (some of them look to be the same? schedule-bg.png and schedule-background.png for example - could we try to achieve the same effect using css gradients instead where possible, loading images would be less efficient although easier to implement)
  • still would like to see commonly used colors inserted into variables in globals/vh-styles.scss and then have Schedule.scss refer to them using the variables instead of the values (e.g. $dark-purple instead of #322660, $secondary-pink instead of #ee99a0)

i can give the countdown issue a look after today's meeting; i think if we can also achieve all the styling we want using css then we won't have to resort to using the countdown-etcetcetc.png images. ideally we'd also want to finish mobile styling too since people will be looking at the website from their phones but let's tackle this one step at a time first

@alicetran1221
Copy link
Collaborator Author

alicetran1221 commented May 22, 2024

Woops sorry, I didn’t commit my changes yet so all those issues you mentioned are fixed, I wanted to wait on pushing while I got info for the concerns I had but I can push later in the morning!

Also will get rid of any extra images and try to use css color gradient for background but we’d still have to use an background image overlay for the stars

Copy link
Collaborator

@alanchangxyz alanchangxyz left a comment

Choose a reason for hiding this comment

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

so code wise this is pretty good, i don't think there are too many things i'd suggest changing here but we should fix them before this is merged; there's also the option of animating parts of the page if y'all wanna try that so just shout/slack if you'd like to give it a shot 🙂

good work 🎉

src/app/components/schedule-card/ScheduleCard.jsx Outdated Show resolved Hide resolved
src/app/data/schedule-info.js Outdated Show resolved Hide resolved
@@ -66,7 +67,7 @@
margin-left: -5px;
border-width: 5px;
border-style: solid;
border-color: transparent transparent #f6d4d4dc transparent;
border-color: transparent transparent $peach transparent;
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe we can just convert this to a border-bottom-color rule since we're only working with the bottom border in this

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

so the peach one should be border-bottom-color, just making sure bc it looks a little funky when i do that:

image

Copy link
Collaborator

Choose a reason for hiding this comment

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

mmm do you mean the black square - i'm leaning towards keeping it for now then but that styling will def have to be overhauled next year so they don't run into a can of worms when styling either

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yea im not sure what to do about that either, everything else has been fixed though

Copy link
Collaborator

Choose a reason for hiding this comment

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

okay we can leave it then LOL. i'll add a note to look into tooltip styling in the future but otherwise we'll [animate and] merge 🥳

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

would it be okay to merge now since animation would be nice but i don't have the time to do it, also merging into main right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

yup! i'll merge it now

src/app/views/schedule/Schedule.jsx Outdated Show resolved Hide resolved
src/app/views/schedule/Schedule.scss Outdated Show resolved Hide resolved
src/app/components/countdown/Countdown.jsx Outdated Show resolved Hide resolved
src/app/components/schedule-card/ScheduleCard.jsx Outdated Show resolved Hide resolved
src/app/components/schedule-card/ScheduleCard.scss Outdated Show resolved Hide resolved
@alanchangxyz alanchangxyz merged commit 3253527 into main May 24, 2024
@alanchangxyz alanchangxyz mentioned this pull request May 25, 2024
@alanchangxyz alanchangxyz linked an issue May 25, 2024 that may be closed by this pull request
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.

Schedule Page
2 participants