-
Notifications
You must be signed in to change notification settings - Fork 22
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
JEDI Page #235
Conversation
cc: @evanzhong as he was interested |
Took a look at this today, love the way the skeleton is shaping up! Do you need any help with ideas for the nav? Lot of content for the JEDI pages and I like what you're doing with the sub nav bar. It took me a while to figure out how to use it, mainly b/c the nav is on the left for the home page, but then shifts up to top of the main body for the other pages |
Yeah, that's a good point! I had initially implemented it this way since that's what Helia asked for in the mockup/ what she wanted from the call we had, but I agree that the transition from the home page -> not the home page is a bit jarring. Definitely welcoming any ideas, I guess what I have in mind is:
Also will eventually QA review this through Design (once some of the content is done), and they might be able to provide some more input too! |
This PR implements per-page SEO tags via [`next-seo`](https://github.com/garmeeh/next-seo). Under-the-hood, the package handles basic meta tags, and OpenGraph URLs; according to the documentation, Twitter now also parses OG data, and thus doesn't generate twitter-specific metadata. I then manually write and apply copy for each page: the home page, about, committees, events, sponsors, and tech gala. We'll have to backport this to other new pages, like the ones introduced in #238 and #235. I chose not to add any default templating or abstract this to a component, since I think the copy differs enough where boilerplate doesn't help us too much. For reviewers - it would be great if you could test this on a variety of social media platforms (Facebook, Twitter, Discord, Instagram. etc.). I've done Discord testing in the #test3 moderator channel of the server, as well as DMing myself on FB messenger. Closes #116, (hopefully) closes #194.
This PR implements per-page SEO tags via [`next-seo`](https://github.com/garmeeh/next-seo). Under-the-hood, the package handles basic meta tags, and OpenGraph URLs; according to the documentation, Twitter now also parses OG data, and thus doesn't generate twitter-specific metadata. I then manually write and apply copy for each page: the home page, about, committees, events, sponsors, and tech gala. We'll have to backport this to other new pages, like the ones introduced in #238 and #235. I chose not to add any default templating or abstract this to a component, since I think the copy differs enough where boilerplate doesn't help us too much. For reviewers - it would be great if you could test this on a variety of social media platforms (Facebook, Twitter, Discord, Instagram. etc.). I've done Discord testing in the #test3 moderator channel of the server, as well as DMing myself on FB messenger. Closes #116, (hopefully) closes #194.
Submitted design QA: https://trello.com/c/9fU9NGn0/372-qa-approval-acm-jedi-website-pages |
Looking to get a general review on what we have here right now. I'd err more on reviewing the coding style / separation of components, rather than tinier elements of the design - Design will be handling that portion in their QA. Things that aren't done yet:
|
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.
Looking good! Most of my review left as comments as usual.
Will do another pass after QA comes back and changes made for that, but barring that looks good to ship.
components/JEDI/AllyshipSpaceCard.js
Outdated
* creates an allyship space card from input parameters, using | ||
* next/image and next/link | ||
*/ | ||
function AllyshipSpaceCard({ title, date, location, description, pic, alt, rsvp, slides }) { |
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.
IIRC we have a component that's similar to this created for the techgala page called ProjectCard
.
I know these two components are functionally different, but thoughts on if they're similar enough to warrant creating an "EventCard" component (like a super class of these two). Might be a good thing to do in preparation for the events page, which I imagine will leverage a similar layout as these.
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.
I see what you're saying, though the styles are quite different on each card. On my end, I'm happy to keep them separate, but also interested to explore creating a generic Card that has a few configurable options (in this case, which side the image goes on, etc.). Just worried that then we'll get way too much required customization. What do you think?
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.
I think what we can do here is leverage the children
prop to get the customization we need. Then for the super class we'd just need to specify and image and where the text should flow (left, right, or below).
The hope is that we can have some generic css that will format the height and width of the children (usually just text) to flow with the image.
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.
Sounds good to me, let's see if I can get that in relatively quickly for this change.
Yup, I think I'll just open a separate PR for that once Vincent's approach (#253) get merged in! Tanaya had a neat addition to get the impact page to slot in nicely to the nav. |
Just did the first pass of design QA suggestions; you can follow the conversation in the trello card. Still waiting on:
Going to re-req a code review with a bias towards maintainability and design feedback. The generalized card is still in the back of my mind, though I may ask to punt it just due to the sheer number of tickets I need to work on right 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.
Took a quick pass at the new commits since my last review, everything looks good to me.
Does design have anything specifically to say about the nav bar? (i.e. how it shifts from the left sidebar to the top)
pages/jedi/index.js
Outdated
<h1 className={styles['hero-title']}>ACM | JEDI</h1> | ||
<p>justice, equity, diversity, inclusion</p> | ||
<p className={styles['hero-subtitle']}>justice, equity, diversity, inclusion</p> |
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.
What does hero
mean
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.
Common SEO / marketing term for above-the-fold images & headers! Here's a spammy-ish article about it.
import { | ||
faDiscord, | ||
faSlack, | ||
} from '@fortawesome/free-brands-svg-icons'; |
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.
Have we ported over all the other icons we use in the website to use font awesome?
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.
Yup!
As-is, Amy was satisfied with it - she did want me to increase the spacing and consider adding an image! |
Going to merge now; Crystal is hitting us with QA after-the-fact. |
This PR implements three pages from Helia's wireframe:
Outside of directly implementing the pages, we:
Open Sans
, includes semi-bold, bold, and bold italicTechnical to-do tracker:
<a>
and button CSSnext/image
#188icons for link buttons!going to punt this as out of scope unless Design requires it for QAprogrammatic datesgoing to punt this as out of scopelocation links?going to punt this as out of scope