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 6 months ago
e26chiu commented 7 months ago
Collaborator
  • 3 News items are displayed on the homepage (MiniNewsCard component)
  • Single News item path: /news/20xx/term/20xx-xx-xx (NewsCard component)
    • Purple header for the date
    • News items of the same date are displayed on the same page
* 3 News items are displayed on the homepage (`MiniNewsCard` component) * Single News item path: `/news/20xx/term/20xx-xx-xx` (`NewsCard` component) * Purple header for the date * News items of the same date are displayed on the same page
e26chiu added 1 commit 7 months ago
n3parikh was assigned by e26chiu 7 months ago
a258wang was assigned by e26chiu 7 months ago
Owner

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.

image

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. ![image](/attachments/77cebd02-48c8-4ad9-97a3-b7ee5b7a3b05)
Poster
Collaborator

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.

image

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:

.card {
  display: -webkit-box;
  -webkit-line-clamp: 3;
  -webkit-box-orient: vertical;  
  overflow: hidden;
}

This property apparently is not supported on IE.

Oh, I see that it doesn't display properly on Microsoft Edge either...

> 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. > > ![image](/attachments/77cebd02-48c8-4ad9-97a3-b7ee5b7a3b05) 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: ```css .card { display: -webkit-box; -webkit-line-clamp: 3; -webkit-box-orient: vertical; overflow: hidden; } ``` This property apparently is not supported on [IE](https://css-tricks.com/almanac/properties/l/line-clamp/). Oh, I see that it doesn't display properly on Microsoft Edge either...
Owner

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:
image

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...

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: ![image](/attachments/e79a1be5-2a35-47fc-a4aa-44f179337b06) 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...
Owner

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.

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.
Owner

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.

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. 🙂

> 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. 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. 🙂
e26chiu added 1 commit 7 months ago
2d27055bf8 truncate markdown to 150 char
e26chiu added 1 commit 7 months ago
70a569ca74 Merge branch 'main' into support-multiple-news-homepage
e26chiu requested review from n3parikh 7 months ago
e26chiu requested review from a258wang 7 months ago
Owner

Just a nitpick, but it looks like the bullet icons are getting cut off on mobile in Chrome:
image

Also interesting to note, for this particular news item, the truncating is not the same in Chrome vs. Firefox:
image
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.

Just a nitpick, but it looks like the bullet icons are getting cut off on mobile in Chrome: ![image](/attachments/e12dbde0-7f3c-4374-8134-671036785d90) Also interesting to note, for this particular news item, the truncating is not the same in Chrome vs. Firefox: ![image](/attachments/e2214ca6-5983-425c-8707-f60e8e6e534b) 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.
a258wang reviewed 6 months ago
border-radius: calc(20rem / 16);
display: -webkit-box;
-webkit-line-clamp: 3;
-webkit-box-orient: vertical;
Owner

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.

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.
a258wang reviewed 6 months ago
<address className={styles.author}>{author}</address>
<div className={styles.content}>{children}</div>
<Link href={permalink}>
<span className={styles.mobileLearnMore}>Learn more</span>
Owner

There don't seem to be any CSS declarations for the class mobileLearnMore in MiniNewsCard.module.css, so is this class still needed here?

There don't seem to be any CSS declarations for the class `mobileLearnMore` in `MiniNewsCard.module.css`, so is this class still needed here?
a258wang reviewed 6 months ago
lib/news.ts Outdated
);
const { content, data: metadata } = matter(raw);
const slugDate = slug.split("-").slice(0, 3).join("-");
let truncatedContent: string = content;
Owner

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 to rawContent.

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 to `rawContent`.
a258wang reviewed 6 months ago
pages/index.tsx Outdated
<MiniNewsCard
{...news.metadata}
date={new Date(news.metadata.date)}
key={news.metadata.date.toString()}
Owner

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?

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?
a258wang reviewed 6 months ago
key={idx}
{...metadata}
date={new Date(metadata.date)}
fit={true}
Owner

It looks like we're passing fit={true} to the NewsCard, however the "fit" styling is declared on the MiniNewsCard. Is this intentional?

It looks like we're passing `fit={true}` to the `NewsCard`, however the "fit" styling is declared on the `MiniNewsCard`. Is this intentional?
Poster
Collaborator

The "fit" style is also declared in NewsCard. I just copied the code from MiniNewsCard. The "fit" style is still important to keep the formatting of the NewsCard though. Since it's a duplicate code, do you think we need to change the way we include the stylesheet or no?

The "fit" style is also declared in `NewsCard`. I just copied the code from `MiniNewsCard`. The "fit" style is still important to keep the formatting of the `NewsCard` though. Since it's a duplicate code, do you think we need to change the way we include the stylesheet or no?
Owner

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 the NewsCard component explaining the purpose of the "fit" styles?

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 the `NewsCard` component explaining the purpose of the "fit" styles?
e26chiu added 2 commits 6 months ago
a258wang reviewed 6 months ago
<div className={styles.page}>
<Title>{["News", `${date}`]}</Title>
<h1>
News: <span>{date}</span>
Owner

Is the span here still necessary?

Is the `span` here still necessary?
a258wang approved these changes 6 months ago
a258wang left a comment
Owner

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 unnecessary span
  • pages/news/.../[date].tsx, line 76 - there's a potentially unnecessary console.log
  • Consider leaving a quick comment about the "fit" style in NewsCard.tsx
    Other than these small details, this PR should be ready to merge!
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 unnecessary `span` - `pages/news/.../[date].tsx`, line 76 - there's a potentially unnecessary `console.log` - Consider leaving a quick comment about the "fit" style in `NewsCard.tsx` Other than these small details, this PR should be ready to merge!
author: string;
children: ReactNode;
permalink: string;
fit?: boolean;
Owner

It might be helpful to leave a comment here explaining what the fit prop is used for.

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);
Owner

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. 🙂

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. 🙂
a258wang approved these changes 6 months ago
a258wang left a comment
Owner

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 unnecessary span
  • pages/news/.../[date].tsx, line 76 - there's a potentially unnecessary console.log
  • Consider leaving a quick comment about the "fit" style in NewsCard.tsx
    Other than these small details, this PR should be ready to merge!
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 unnecessary `span` - `pages/news/.../[date].tsx`, line 76 - there's a potentially unnecessary `console.log` - Consider leaving a quick comment about the "fit" style in `NewsCard.tsx` Other than these small details, this PR should be ready to merge!
e26chiu added 1 commit 6 months ago
215fbfa098 add comment & fix misc stuff
e26chiu merged commit 31c81f8620 into main 6 months ago

Reviewers

n3parikh was requested for review 7 months ago
a258wang approved these changes 6 months ago
continuous-integration/drone/push Build is passing
The pull request has been merged as 31c81f8620.
Sign in to join this conversation.
No Milestone
No project
3 Participants
Notifications
Due Date

No due date set.

Dependencies

This pull request currently doesn't have any dependencies.

Loading…
There is no content yet.