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

[next] 1358 - MainPage (index.js) #1531

Merged
merged 7 commits into from
Jan 20, 2021

Conversation

PedroFonsecaDEV
Copy link
Contributor

@PedroFonsecaDEV PedroFonsecaDEV commented Dec 15, 2020

Issue This PR Addresses

Fixes #1358 - [next] Port MainPage.

Type of Change

PORT TO NEXT.JS

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

Description

The MainPage.tsx is done; I had to create some basic components to have it.

It seems to me that we don't need the PageBase component now.

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)

@humphd
Copy link
Contributor

humphd commented Jan 15, 2021

Just starting to look at this, and wondering if you have tips on how to test this? @PedroFonsecaDEV

import Head from 'next/head';
import styles from '../styles/Home.module.css';
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like we haven't got CSS figured out yet, is that happening in a follow-up?

Copy link
Contributor Author

@PedroFonsecaDEV PedroFonsecaDEV Jan 16, 2021

Choose a reason for hiding this comment

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

File: telescope/src/frontend/gatsby/src/pages/index.js

import React from 'react';

import PageBase from './PageBase';
import Banner from '../components/Banner';
import Posts from '../components/Posts';
import BackToTopButton from '../components/BackToTopButton';

export default function IndexPage() {
  return (
    <PageBase title="Home">
      <Banner />
      <BackToTopButton />
      <main className="main">
        <Posts />
      </main>
    </PageBase>
  );
}

We don't have a CSS in this component. In our Gatsby, the PageBase component holds the global CSS and SEO.
With Next.js, we may have to break the PageBase. Reading this Next.js doc I understood that the "global CSS" would be defined on the file: next/src/pages/_app.tsx. But I would appreciate feedback on this approach.

Note about SEO: I'm not sure if we could add our SEO to _app.tsx or if we need to add it to each page.

Copy link
Contributor

Choose a reason for hiding this comment

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

@PedroFonsecaDEV : We can defined global CSS in MUI theme as well. I think that would be a better approach to have global styling in the theme.

Copy link
Contributor

Choose a reason for hiding this comment

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

For global styling, the work happening in #1549 is probably the right approach.

import BackToTopButton from '../components/BackToTopButton/BackToTopButton';
import Posts from '../components/Posts/Posts';

const MainPage: FC = () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

if the name of this file is index.tsx, it is understood that this component should render the Home page. For consistency, can we keep the name of this component as Home instead of MainPage since the navbar has it as Home as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure.
In our Gatsby we export this component as IndexPage.
and
In our Port issue we refer to this component as Main Page.

I will change it to Home.

@humphd humphd added this to the 1.5 Release milestone Jan 15, 2021
@PedroFonsecaDEV
Copy link
Contributor Author

@humphd

Just starting to look at this, and wondering if you have tips on how to test this? @PedroFonsecaDEV

I use the Netlify deployment to test it here on GitHub.

@tonyvugithub
Copy link
Contributor

I am still leaning toward keeping the PageBase component for quick migration. The least adjustment we need to make the better. How you guys think?

@PedroFonsecaDEV
Copy link
Contributor Author

@tonyvugithub

I am still leaning toward keeping the PageBase component for quick migration. The least adjustment we need to make the better. How you guys think?

I'm working on the pageBase problem (#1548). Probably I will break It into small PRs. And the first one is gonna be the pageBase solution.


const BackToTopButton: FC = () => <h3>BackToTopButton Component</h3>;

export default BackToTopButton;
Copy link
Contributor

Choose a reason for hiding this comment

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

In a few of our current PRs we're adding an index.ts file for easier imports, see here for an example, can you convert BackToTopButton, Banner, and Posts into that? The imports will need to get changed after too 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

This is a great point to raise, @chrispinkney. Here are my thoughts:

  • JSX (and for TypeScript, TSX) is a non-standard extension of JavaScript
  • Some people, especially outside the React ecosystem, don't like using .js for a file that isn't standard JavaScript (or .ts for TypeScript).
  • Transpilers (i.e., Babel, which translates JSX to pure JS) doesn't care what you call it, and at runtime, there is no-such thing as JSX: it all gets converted to pure functions.
  • The general advice with transpiled code is to import it without the extension: import Foo from './foo' without any extension, and let the module system figure it out at build- vs. run-time.
  • We are using React and next.js, so the vast majority of files are likely to contain JSX (i.e., it's not an edge case we need to worry about).

I think that another reason we used the index.js approach to do the final export was that we also wanted to put CSS files in the same scope:

/Foo
    index.js    <---- takes care of combining the JSX and CSS, and provides the default export for Foo
    Foo.jsx
    Foo.css

Later, we moved to Material UI, and with it, CSS-in-JS. Next.js doesn't really like global CSS anyway (they either want you to use CSS Modules or CSS-in-JS).

In summary, I think we can ditch the index.js export files completely. If we follow the patterns in the next.js TypeScript example, in this PR I would suggest we do this:

src/frontend/next/src/components/BackToTopButton/BackToTopButton.tsx becomes src/frontend/next/src/components/BackToTopButton.tsx (i.e., drop the directory and use tsx).

Copy link
Contributor

Choose a reason for hiding this comment

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

Filed #1553 for follow-up.

Copy link
Contributor

Choose a reason for hiding this comment

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

Filed #1554 for follow-up.

@tonyvugithub
Copy link
Contributor

I wonder if we can merge this now or wait after we merge BackTopTopBUtton, Header and Posts to merge it. What would be a better approach here. This looks good to go though


const BackToTopButton: FC = () => <h3>BackToTopButton Component</h3>;

export default BackToTopButton;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a great point to raise, @chrispinkney. Here are my thoughts:

  • JSX (and for TypeScript, TSX) is a non-standard extension of JavaScript
  • Some people, especially outside the React ecosystem, don't like using .js for a file that isn't standard JavaScript (or .ts for TypeScript).
  • Transpilers (i.e., Babel, which translates JSX to pure JS) doesn't care what you call it, and at runtime, there is no-such thing as JSX: it all gets converted to pure functions.
  • The general advice with transpiled code is to import it without the extension: import Foo from './foo' without any extension, and let the module system figure it out at build- vs. run-time.
  • We are using React and next.js, so the vast majority of files are likely to contain JSX (i.e., it's not an edge case we need to worry about).

I think that another reason we used the index.js approach to do the final export was that we also wanted to put CSS files in the same scope:

/Foo
    index.js    <---- takes care of combining the JSX and CSS, and provides the default export for Foo
    Foo.jsx
    Foo.css

Later, we moved to Material UI, and with it, CSS-in-JS. Next.js doesn't really like global CSS anyway (they either want you to use CSS Modules or CSS-in-JS).

In summary, I think we can ditch the index.js export files completely. If we follow the patterns in the next.js TypeScript example, in this PR I would suggest we do this:

src/frontend/next/src/components/BackToTopButton/BackToTopButton.tsx becomes src/frontend/next/src/components/BackToTopButton.tsx (i.e., drop the directory and use tsx).

Copy link
Contributor

@humphd humphd left a comment

Choose a reason for hiding this comment

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

I think we can merge this, and then replace things when the other components are done. It would be good to get a main page that works.

Copy link
Member

@manekenpix manekenpix left a comment

Choose a reason for hiding this comment

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

LGTM 👍

image

@HyperTHD HyperTHD merged commit c014c18 into Seneca-CDOT:master Jan 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: nextjs Nextjs related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[next] Main page, see src/frontend/src/pages/index.js
7 participants