Fix #1459: Added Post and Posts to NextJS #1504

rogercyyu · 2020-12-08T04:23:22Z

Issue This PR Addresses

This issue address #1459.

Type of Change

Bugfix: Change which fixes an issue
New Feature: Change which adds functionality
Documentation Update: Change which improves documentation
UI: Change which improves UI

Description

This port Post/ and Posts/ to Next,js and converts it to TypeScript (most i think?).

Tested it by commenting out things that aren't implemented yet. So works mostly.
Requires a lot more components that haven't been implemented yet.
Moved telescope-post-content.css to \styles.
Probably some bugs yet to be discovered.

Thoughts?

Thank you for your time and review.

Checklist

Quality: This PR builds and passes our npm test and works locally
Tests: This PR includes thorough tests or an explanation of why it does not
Screenshots: This PR includes screenshots or GIFs of the changes made or an explanation of why it does not (if applicable)
Documentation: This PR includes updated/added documentation to user exposed functionality or configuration variables are added/changed or an explanation of why it does not(if applicable)

rogercyyu · 2020-12-08T04:29:00Z

src/frontend/next/src/components/Post/Post.tsx

+import { Box, Grid, Typography, ListSubheader, createStyles } from '@material-ui/core';
+
+// For style, need to add:
+// import '../styles/telescope-post-content.css';


Typo: This statement needs to be added to index.tsx to work properly.

cc @tonyvugithub, can you take over the global styling we need? We should get an issue filed to move the telescope-post-content.css file into our global css for next.js.

@humphd : I believe @PedroFonsecaDEV is currently working on the global.css file in #1548

Also can we convert the css file of Post to JSS to keep it consistent with the rest of the components? And then we don't have to worry about moving the css to global.css

I don't think we can, or we can't do it easily.

The way we load and display posts now is to send raw HTML and inject it as innerHTML. This means we can't rely on module scoped CSS for a component, since it isn't really a component.

It might be possible down the road to use something like Server Components, but they are still very new. https://addyosmani.com/blog/react-server-components/ has a good discussion. This is a research problem we could tackle on the way to 2.0.

For now, we should probably port it as is.

OK, left a comment in #1548.

You can remove this comment now, @PedroFonsecaDEV is doing it in another PR in the global stylesheet.

tonyvugithub · 2021-01-16T14:05:31Z

@rogercyyu : Hi there, we are in the process of reviewing these migrated to Next PRs. Just wonder if you are still interested to work on this as we would like you to make some changes to your PR. Please let us know in case you decided to drop this then we can assign the issue to others. Thank you.

rogercyyu · 2021-01-16T14:27:13Z

@tonyvugithub I am happy to make some changes to this PR. Thanks

tonyvugithub · 2021-01-16T16:53:31Z

Got it. Thanks. We keep you posted about the progress.

humphd · 2021-01-17T16:39:02Z

src/frontend/next/src/components/Post/Post.tsx

@@ -0,0 +1,210 @@
+import React, { useRef, useState } from 'react';


You don't need to import React in next.js, but the other two are fine.

humphd · 2021-01-17T16:40:18Z

src/frontend/next/src/components/Post/Post.tsx

+import { Box, Grid, Typography, ListSubheader, createStyles } from '@material-ui/core';
+
+// For style, need to add:
+// import '../styles/telescope-post-content.css';


cc @tonyvugithub, can you take over the global styling we need? We should get an issue filed to move the telescope-post-content.css file into our global css for next.js.

humphd · 2021-01-17T16:41:14Z

src/frontend/next/src/components/Post/Post.tsx

+// For style, need to add:
+// import '../styles/telescope-post-content.css';
+
+interface Props {


The next.js docs/examples on using TypeScript suggest using type vs. interface for Props.

humphd · 2021-01-17T16:42:28Z

src/frontend/next/src/components/Post/Post.tsx

+  // We need a ref to our post content, which we inject into a <section> below.
+  const sectionEl = useRef(null);
+  // Grab the post data from our backend so we can render it
+  const getData = async (url: string) => {


We should file an issue to switch this to use useSWR instead.

humphd · 2021-01-17T16:43:53Z

src/frontend/next/src/components/Post/Post.tsx

+  postUrl: string;
+}
+
+interface DataItem {


This is really a Post vs. DataItem. We probably should create something like this: https://github.com/vercel/next.js/tree/canary/examples/with-typescript/interfaces and move interface Feed {...} and interface Post {...} in there, then import them into this file so we can use them.

humphd · 2021-01-17T16:46:22Z

src/frontend/next/src/components/Post/Post.tsx

+  html: string;
+}
+
+const useStyles = makeStyles((theme) =>


Typing needed on theme, see https://github.com/mui-org/material-ui/blob/master/examples/nextjs-with-typescript/src/ProTip.tsx#L15

humphd · 2021-01-18T15:55:02Z

src/frontend/next/src/components/Post/Post.tsx


-interface DataItem {
+type DataItem = {


No. The difference here between Props, which is done correctly, and this (which isn't) is that Props is internal to this code. Whereas, we'll share the definition of Feed and Post throughout the frontend.

We need to create a folder at the same level as components and include an index.ts file, like this: https://github.com/vercel/next.js/tree/canary/examples/with-typescript/interfaces

Inside it, you can export two types:

export type Feed = { link: string author: string } export type Post { feed: Feed id: string post: string title: string updated: string url: string html: string }

Then you can import and use them here.

Thank you for the detail feedback

humphd · 2021-01-19T02:42:23Z

src/frontend/next/src/components/Post/Post.tsx

+import { useRef, useState } from 'react';
+
+import useSWR from 'swr';
+import 'highlight.js/styles/github.css';


next.js won't let us do this, we'll have to load this globally, see https://nextjs.org/docs/basic-features/built-in-css-support.

humphd · 2021-01-19T02:44:15Z

src/frontend/next/src/components/Post/Post.tsx

+    return res.json();
+  };
+
+  const { data: post, error } = useSWR<PostType>(postUrl, getData);


It looks like you have both a fetch and a useSWR way in the same file. Can we just do useSWR?

Do you mean, change to:

// Grab the post data from our backend so we can render it const fetcher = (url: string) => fetch(url).then((r) => r.json()); const { data: post, error } = useSWR<Post>(postUrl, fetcher);

?

Right, you don't need to fetch() and useSWR(), they accomplish the same thing, and the latter is better (it will cache it for offline use).

humphd · 2021-01-19T02:45:28Z

src/frontend/next/src/components/Posts/LoadAutoScroll.tsx

+
+function LoadAutoScroll({ onScroll }: Scroll) {
+  const classes = useStyles();
+  const $buttonRef = useRef(null);


See https://github.com/typescript-cheatsheets/react#forwardrefcreateref

Not sure, if I fixed this correctly.

humphd · 2021-01-19T02:46:03Z

src/frontend/next/src/components/Posts/Posts.tsx

+import { usePrevious } from 'react-use';
+import SentimentDissatisfiedRoundedIcon from '@material-ui/icons/SentimentDissatisfiedRounded';
+import Timeline from './Timeline';
+// TODO: Change these to new ones whenever import to NextJS


I've got these finished and merged now, if you want to rebase.

humphd · 2021-01-19T02:46:53Z

src/frontend/next/src/components/Posts/Timeline.tsx

+import useSiteMetaData from '../../hooks/use-site-metadata';
+
+type Props = {
+  pages: Array<any> | undefined;


This will be an Array of Post objects, so we should be able to type it with your interface.

humphd · 2021-01-19T02:47:16Z

src/frontend/next/src/interfaces/index.tsx

+  author: string;
+};
+
+export type PostType = {


Drop the *Type suffix.

humphd · 2021-01-19T02:47:41Z

src/frontend/next/src/pages/_app.tsx

@@ -1,5 +1,7 @@
 import { AppProps } from 'next/app';
 import '../styles/globals.css';
+import '../styles/empty.css';
+import '../styles/Post.css';


These won't work here, needs to be globally imported.

Whoops, those are mistakes leftover from testing.

humphd · 2021-01-20T14:23:52Z

src/frontend/next/src/components/Post/Post.tsx

+import { Box, Grid, Typography, ListSubheader, createStyles } from '@material-ui/core';
+
+// For style, need to add:
+// import '../styles/telescope-post-content.css';


You can remove this comment now, @PedroFonsecaDEV is doing it in another PR in the global stylesheet.

humphd · 2021-01-20T14:25:04Z

src/frontend/next/src/components/Post/Post.tsx

+  return `Last Updated ${formatted}`;
+};
+
+const Post = ({ postUrl }: Props) => {


We can't have 2 things named Post. You are importing a Post type, and now defining another Post as the component. You could call this PostComponent if you want.

humphd · 2021-01-20T14:26:21Z

src/frontend/next/src/components/Post/Post.tsx

+const Post = ({ postUrl }: Props) => {
+  const classes = useStyles();
+  // We need a ref to our post content, which we inject into a <section> below.
+  const sectionEl = useRef(null);


We should type this: useRef<HTMLSectionElement>(); I think.

humphd · 2021-01-20T14:26:35Z

src/frontend/next/src/components/Post/Post.tsx

+  // Grab the post data from our backend so we can render it
+  const fetcher = (url: string) => fetch(url).then((r) => r.json());
+
+  const { data: post, error } = useSWR<PostType>(postUrl, fetcher);


Post vs. PostType

Also, I don't think we need this fetcher since it's the default. You can drop the second arg and delete line 110, 111.

humphd · 2021-01-20T14:28:08Z

src/frontend/next/src/components/Post/Post.tsx

+    return (
+      <Box className={classes.root} boxShadow={2}>
+        <ListSubheader className={classes.header}>
+          <AdminButtons />


You need to import AdminButtons to use it here. It's happening in #1515, which we need to land to get it here.

humphd · 2021-01-20T14:28:39Z

src/frontend/next/src/components/Post/Post.tsx

+
+        <Grid container justify="center">
+          <Grid item className={classes.spinner}>
+            <Spinner animation="border" variant="light">


You need Spinner, see #1479

humphd · 2021-01-20T14:29:34Z

src/frontend/next/src/components/Posts/Timeline.tsx

@@ -0,0 +1,58 @@
+import { FC } from 'react';


You can drop this.

humphd · 2021-01-20T14:29:45Z

src/frontend/next/src/components/Posts/Timeline.tsx

+import useSiteMetaData from '../../hooks/use-site-metadata';
+
+type Props = {
+  pages: Array<typeof Post> | undefined;


Array<Post>

src/frontend/next/src/pages/_app.tsx

humphd · 2021-01-20T14:31:08Z

src/frontend/next/src/styles/telescope-post-content.css

@@ -0,0 +1,127 @@
+.telescope-post-content {


This file can be removed, @PedroFonsecaDEV is doing it globally.

PedroFonsecaDEV · 2021-01-21T07:49:57Z

This PR looks good.
Still depending on #1515.
Maybe a good option would be to create a simple AdminButtons on the right folder to merge this one? Like a skeleton.
Something like this:

const AdminButtons = () => <h3> AdminButtons<h3>

humphd

Thanks for jumping on these fixes, we really need this PR to land soon, so the new frontend can be tested with our backend data.

src/frontend/next/src/components/Post/PostComponent.tsx

humphd · 2021-01-21T14:12:06Z

src/frontend/next/src/components/Posts/LoadAutoScroll.tsx

+    observer.observe($buttonRef.current!);
+
+    return () => {
+      observer.unobserve($buttonRef.current!);


Need to fix the eslint warning you have on $buttonRef.current. cc @birtony, who just fixed this kind of thing for #1227.

humphd · 2021-01-21T14:12:48Z

src/frontend/next/src/components/Posts/Posts.tsx

@@ -0,0 +1,107 @@
+import { useState, useEffect } from 'react';


This file can be renamed to src/frontend/next/src/components/Posts/index.tsx

humphd · 2021-01-21T14:13:08Z

src/frontend/next/src/components/Posts/Posts.tsx

+import useSiteMetaData from '../../hooks/use-site-metadata';
+import useFaviconBadge from '../../hooks/use-favicon-badge';
+
+interface PostData {


Why not use a proper Post type from our interfaces?

* Fixed eslint issues (hopefully) & PR changes

rogercyyu · 2021-01-21T23:45:02Z

Thanks for jumping on these fixes, we really need this PR to land soon, so the new frontend can be tested with our backend data.

Your welcome. Happy to help!

humphd · 2021-01-21T23:45:52Z

@rogercyuu is this ready for re-review then?

rogercyyu · 2021-01-21T23:48:09Z

@humphd yup

tonyvugithub · 2021-01-22T01:58:17Z

src/frontend/next/src/components/Posts/Timeline.tsx

+
+  // Iterate over all the pages (an array of arrays) and then convert all post
+  // elements to <Post>
+  const postsTimeline = pages.map((page: Post[]): any =>


@humphd : What does this mean in TS? I thought page already have type Post[]. The return? If so it would return a list of , no idea how to express that in syntax

tonyvugithub · 2021-01-22T01:59:43Z

src/frontend/next/src/components/Posts/index.tsx

+import Timeline from './Timeline';
+import useSiteMetaData from '../../hooks/use-site-metadata';
+import useFaviconBadge from '../../hooks/use-favicon-badge';
+import { Post } from '../../interfaces/index';


I think index work here as Posts have dependent component. Instead of doing /Posts/Posts.tsx, no?

tonyvugithub · 2021-01-22T02:22:29Z

src/frontend/next/src/components/Post/index.tsx

@@ -0,0 +1,190 @@
+import { useRef, useState } from 'react';


I feel like you can just move this Posts.tsx component inside the Posts folder.

tonyvugithub · 2021-01-22T02:22:34Z

src/frontend/next/src/components/Posts/Timeline.tsx

+
+  // Iterate over all the pages (an array of arrays) and then convert all post
+  // elements to <Post>
+  const postsTimeline = pages.map((page: Post[]): any =>


@rogercyyu : I would say keep any for now.

src/frontend/next/src/components/Posts/Timeline.tsx

PedroFonsecaDEV

Well Done!

There are somet things that need to be fixed

src/frontend/next/src/components/Posts/index.tsx

PedroFonsecaDEV · 2021-01-22T04:27:06Z

@rogercyyu, how did you tested the PR?
We are experiencing some issues regarding the infinity scroll.

rogercyyu · 2021-01-22T04:41:42Z

@PedroFonsecaDEV hmmm works fine on my end. Not sure what's happening. I used the npm run develop:next. Is the build or serve one not working?

PedroFonsecaDEV · 2021-01-22T04:52:44Z

@PedroFonsecaDEV hmmm works fine on my end. Not sure what's happening. I used the npm run develop:next. Is the build or serve one not working?

The infinity scroll starts an infinite loop after 4+ posts loads.

humphd · 2021-01-22T16:57:37Z

I want to land this today, what's missing that needs to happen? If there's a scrolling bug, let's file that for follow-up vs holding this up. Please update ASAP.

humphd · 2021-01-22T16:57:58Z

Looks like a rebase is needed for sure...

humphd

I think this is good for 1.5, and we can fix anything that comes of it in follow-ups.

src/frontend/next/src/components/AdminButtons.tsx

humphd · 2021-01-22T18:27:47Z

src/frontend/next/src/components/Posts/index.tsx

+  const [currentPostId, setCurrentPostId] = useState<string | null>();
+
+  const { data, size, setSize, error } = useSWRInfinite<Post[]>(
+    (index: number) => `${telescopeUrl}/posts?page=${index + 1}`,


When I load https://deploy-preview-1504--telescope-next.netlify.app/ it looks like telescopeUrl is undefined on Netlify, it is loading this URL:

https://deploy-preview-1504--telescope-next.netlify.app/undefined/posts?page=1

Filed #1589 to deal with this.

vercel bot deployed to Preview December 8, 2020 04:23 View deployment

rogercyyu commented Dec 8, 2020

View reviewed changes

rogercyyu added area: front-end area: nextjs Nextjs related issues labels Dec 8, 2020

rogercyyu self-assigned this Dec 8, 2020

humphd requested changes Jan 17, 2021

View reviewed changes

humphd mentioned this pull request Jan 17, 2021

[next] Port global config of 1)pages/styles 2)src/frontend/src/pages/PageBase.js 3)src/frontend/src/styles/globals.css 4)styles/telescope-post-content.css 5)highlight.js/styles/github.css #1548

Closed

vercel bot deployed to Preview January 17, 2021 19:06 View deployment

humphd requested changes Jan 18, 2021

View reviewed changes

rogercyyu force-pushed the issue-1459 branch from 188e7b8 to 34135fd Compare January 19, 2021 02:28

vercel bot deployed to Preview January 19, 2021 02:28 View deployment

humphd requested changes Jan 19, 2021

View reviewed changes

PedroFonsecaDEV mentioned this pull request Jan 19, 2021

[next] Item 2 and 4 related to /styles/global.css of Issue #1548 #1562

Merged

8 tasks

vercel bot deployed to Preview January 20, 2021 00:21 View deployment

humphd requested review from tonyvugithub and humphd January 20, 2021 14:23

humphd requested changes Jan 20, 2021

View reviewed changes

rogercyyu force-pushed the issue-1459 branch from 563e1a7 to 2a2d2b7 Compare January 21, 2021 04:24

vercel bot deployed to Preview January 21, 2021 04:25 View deployment

humphd requested changes Jan 21, 2021

View reviewed changes

Port Post & Posts

72cc89c

* Fixed eslint issues (hopefully) & PR changes

rogercyyu force-pushed the issue-1459 branch from 2a2d2b7 to 72cc89c Compare January 21, 2021 23:43

vercel bot deployed to Preview January 21, 2021 23:43 View deployment

vercel bot deployed to Preview January 22, 2021 02:12 View deployment

tonyvugithub suggested changes Jan 22, 2021

View reviewed changes

src/frontend/next/src/components/Posts/Timeline.tsx Show resolved Hide resolved

Moved Post/index.tsx -> Posts/Post.tsx & fixed TS error

2ce6ef7

vercel bot deployed to Preview January 22, 2021 03:02 View deployment

PedroFonsecaDEV previously approved these changes Jan 22, 2021

View reviewed changes

manekenpix requested a review from PedroFonsecaDEV January 22, 2021 03:56

PedroFonsecaDEV requested changes Jan 22, 2021

View reviewed changes

src/frontend/next/src/components/Posts/index.tsx Show resolved Hide resolved

humphd added this to the 1.5 Release milestone Jan 22, 2021

rogercyyu added 2 commits January 22, 2021 12:13

Fixed eslint warning

176c429

Merge remote-tracking branch 'origin/master' into issue-1459

85032d7

vercel bot deployed to Preview January 22, 2021 17:26 View deployment

rogercyyu requested review from tonyvugithub, humphd and PedroFonsecaDEV January 22, 2021 17:57

humphd approved these changes Jan 22, 2021

View reviewed changes

src/frontend/next/src/components/AdminButtons.tsx Show resolved Hide resolved

manekenpix approved these changes Jan 22, 2021

View reviewed changes

humphd mentioned this pull request Jan 22, 2021

telescopeUrl env variable not set properly in Netlify builds #1589

Closed

humphd reviewed Jan 22, 2021

View reviewed changes

humphd merged commit 8f22e42 into Seneca-CDOT:master Jan 22, 2021

This was referenced Jan 22, 2021

fix: #1494 Port AdminButton from gatsby to next.js #1515

Closed

[next] Infinite Scrolling of Posts is broken #1595

Closed

PedroFonsecaDEV mentioned this pull request Jan 29, 2021

[next] Port LoadAutoScroll component to NextJS #1540

Closed

humphd mentioned this pull request Feb 2, 2021

[next] Port Timeline to NextJS #1543

Closed

		@@ -0,0 +1,210 @@
		import React, { useRef, useState } from 'react';

		@@ -0,0 +1,107 @@
		import { useState, useEffect } from 'react';

Fix #1459: Added Post and Posts to NextJS #1504

Fix #1459: Added Post and Posts to NextJS #1504

Conversation

rogercyyu commented Dec 8, 2020

Issue This PR Addresses

Type of Change

Description

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tonyvugithub commented Jan 16, 2021

rogercyyu commented Jan 16, 2021

tonyvugithub commented Jan 16, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PedroFonsecaDEV commented Jan 21, 2021 • edited Loading

humphd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rogercyyu commented Jan 21, 2021

humphd commented Jan 21, 2021

rogercyyu commented Jan 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PedroFonsecaDEV left a comment

Choose a reason for hiding this comment

PedroFonsecaDEV commented Jan 22, 2021

rogercyyu commented Jan 22, 2021 • edited Loading

PedroFonsecaDEV commented Jan 22, 2021

humphd commented Jan 22, 2021

humphd commented Jan 22, 2021

humphd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PedroFonsecaDEV commented Jan 21, 2021 •

edited

Loading

rogercyyu commented Jan 22, 2021 •

edited

Loading