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

Feat/rererereredesign2 #105

Merged
merged 11 commits into from
Sep 12, 2023
Merged

Feat/rererereredesign2 #105

merged 11 commits into from
Sep 12, 2023

Conversation

zhamic7
Copy link
Contributor

@zhamic7 zhamic7 commented Sep 5, 2023

Summary

Closes #

Test Plan

@bliutech bliutech closed this Sep 5, 2023
@bliutech bliutech reopened this Sep 5, 2023
@bliutech bliutech self-requested a review September 5, 2023 00:14
Copy link
Member

@bliutech bliutech 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! Couple points of feedback.

Spacing looks a bit cramped below. Would this be something that we could adjust? Also, the corresponding height and width for images seems a bit cramped as well.
image

I noticed you opted to use the styling of the events page. Is this there a reason for this? Could I suggest keeping styling separate since I assume similar to the above problem, there may be classes or styles that differ especially spacing wise between the two pages.

@bliutech bliutech linked an issue Sep 5, 2023 that may be closed by this pull request
3 tasks
@zhamic7
Copy link
Contributor Author

zhamic7 commented Sep 5, 2023

It is more advantageous to uniformize styles in code so we can avoid visual inconsistencies. The problem with image sizing is due to a discrepancy in how events.tsx and archive.tsx size images of arbitrary dimensions-- I will fix that. The archive popup is essentially the event popup with an extra section, so I can just adjust the spacing between old/new sections if the original spacing of the event popup is fine. I don't think either of these problems warrants separate styling.

@bliutech
Copy link
Member

bliutech commented Sep 5, 2023

So I will use /blog as a reference point. In terms of those cards, it makes sense to stylize them separately just due to the fact that there is additional content. While these pages are both events, do you think it's worthwhile tying these two together? This may cause issues down the line when trying to stylize things individually. I will let you make the final call on this.

@bliutech
Copy link
Member

bliutech commented Sep 5, 2023

Also, quick suggestion. Are there better SVG assets that are more similar in style to the pure white assets we use on the home page? This may make things fit more in style.

@zhamic7
Copy link
Contributor Author

zhamic7 commented Sep 5, 2023

If we want to drastically change archive/event cards in style, it would be as much work to rewrite some ArchivePopUp.module.scss as it would be to add it. Since we've already decided to use the same card format for now, there's no reason to not maintain concision. Blog cards are inherently different in content, and thus similarities between blog and archive/event cards are negligible in the first place; archived events are derived from events content-wise.

@zhamic7
Copy link
Contributor Author

zhamic7 commented Sep 11, 2023

Revisions of archive page look good. May want to change the font size of "No Recording"/"No Slides" to that of time/location on event cards (i.e. smaller).

@bliutech bliutech self-requested a review September 12, 2023 00:02
Copy link
Member

@bliutech bliutech left a comment

Choose a reason for hiding this comment

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

Approved with @zhamic7's feedback!

@bliutech bliutech merged commit c2f3c0d into main Sep 12, 2023
@bliutech bliutech deleted the feat/rererereredesign2 branch September 12, 2023 00:02
burturt pushed a commit that referenced this pull request Jan 10, 2024
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.

feat: /archive Page
2 participants