Add Multiple News Items on Homepage and Single News Item Page (Closes #281) #390
Merged
e26chiu
merged 6 commits from support-multiple-news-homepage
into main
1 year ago
Loading…
Reference in new issue
There is no content yet.
Delete Branch 'support-multiple-news-homepage'
Deleting a branch is permanent. It CANNOT be undone. Continue?
MiniNewsCard
component)/news/20xx/term/20xx-xx-xx
(NewsCard
component)When I go to the staging site (https://csclub.uwaterloo.ca/~a3thakra/csc/support-multiple-news-homepage/) I see multiple items, but expanded rather than summaries.
What browser are you using? Is it Internet Explorer? I implemented the truncating bit with Amy's proposed solution which was using css'
-webkit-line-clamp
property:This property apparently is not supported on IE.
Oh, I see that it doesn't display properly on Microsoft Edge either...
This is interesting - the truncating doesn't seem to work on Chrome, but it does work in Firefox.
Screenshot from the staging website in Firefox:

When I was experimenting, I was able to get
-webkit-line-clamp
to work in Chrome by applying the class directly to the first paragraph of the news item, though this is not ideal since sometimes the first paragraph is really short and we want to see a bit of the second paragraph as well. So I'm not quite sure what the best solution is...It looks like
-webkit-line-clamp
is a non standard property that has some issues, according to the MDN docs (see https://developer.mozilla.org/en-US/docs/Web/CSS/-webkit-line-clamp), so maybe we should just avoid it?One alternative approach could be to do the clamping on the "server"/build time (ie. in the code that fetches the content from file) instead. That way we don't need to do any clamping on client side.
This way also makes it easier to add custom short descriptions if we choose to do so later.
I agree that this will probably be the best solution. I personally think the cleanest approach would be to create a new function specifically for fetching the truncated version of the news (eg.
getShortNewsBySlug
or something), but I'll leave the final decision up to Emily. 🙂Just a nitpick, but it looks like the bullet icons are getting cut off on mobile in Chrome:

Also interesting to note, for this particular news item, the truncating is not the same in Chrome vs. Firefox:

I suspect it has something to do with the list? This isn't a merge-blocking thing, but it would be nice to figure out why it's happening, at least.
border-radius: calc(20rem / 16);
display: -webkit-box;
-webkit-line-clamp: 3;
-webkit-box-orient: vertical;
Ahh I may have found the answer to my question about the differences in appearance between Chrome vs. Firefox... we should probably remove these experimental CSS properties.
<address className={styles.author}>{author}</address>
<div className={styles.content}>{children}</div>
<Link href={permalink}>
<span className={styles.mobileLearnMore}>Learn more</span>
There don't seem to be any CSS declarations for the class
mobileLearnMore
inMiniNewsCard.module.css
, so is this class still needed here?);
const { content, data: metadata } = matter(raw);
const slugDate = slug.split("-").slice(0, 3).join("-");
let truncatedContent: string = content;
Nitpick: We could probably use a ternary operator here instead of an if statement. This is mostly my opinion, but in general I feel like if statements are preferable for conditional behaviour, whereas ternary operators are preferable for conditional assignment.
An even nitpickier nitpick: It might be nice to name this variable
content
, since it might not always be truncated. To make this work, we could change the name of the variable on line 77 torawContent
.<MiniNewsCard
{...news.metadata}
date={new Date(news.metadata.date)}
key={news.metadata.date.toString()}
On the off chance that multiple news items have the same date, maybe we can make the key
news.metadata.date.toString() + idx
or something like that?key={idx}
{...metadata}
date={new Date(metadata.date)}
fit={true}
It looks like we're passing
fit={true}
to theNewsCard
, however the "fit" styling is declared on theMiniNewsCard
. Is this intentional?The "fit" style is also declared in
NewsCard
. I just copied the code fromMiniNewsCard
. The "fit" style is still important to keep the formatting of theNewsCard
though. Since it's a duplicate code, do you think we need to change the way we include the stylesheet or no?Ah, I think what's happening is we're using the
NewsCard
component on both the Home page and on the New Archive pages - on the Home page we don't want the "fit" styles, however on the Archive pages we want them. Perhaps we can leave a quick comment in theNewsCard
component explaining the purpose of the "fit" styles?<div className={styles.page}>
<Title>{["News", `${date}`]}</Title>
<h1>
News: <span>{date}</span>
Is the
span
here still necessary?Great job Emily! To summarize my feedback (I clicked some weird buttons by accident so idk if you can see all the comments):
pages/news/.../[date].tsx
, line 42 - there's a potentially unnecessaryspan
pages/news/.../[date].tsx
, line 76 - there's a potentially unnecessaryconsole.log
NewsCard.tsx
Other than these small details, this PR should be ready to merge!
author: string;
children: ReactNode;
permalink: string;
fit?: boolean;
It might be helpful to leave a comment here explaining what the
fit
prop is used for.const news = await Promise.all(
slugs.map((slug) => getNewsBySlug(year, term, slug))
);
console.log(slugs);
Is there a reason why we want to console log here? If it's just for debugging/testing purposes, we should probably get rid of it before merging. 🙂
Great job Emily! To summarize my feedback (I clicked some weird buttons by accident so idk if you can see all the comments):
pages/news/.../[date].tsx
, line 42 - there's a potentially unnecessaryspan
pages/news/.../[date].tsx
, line 76 - there's a potentially unnecessaryconsole.log
NewsCard.tsx
Other than these small details, this PR should be ready to merge!
31c81f8620
into main 1 year agoReviewers
31c81f8620
.