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

Port Footer to NextJS #1526

Merged
merged 2 commits into from Jan 18, 2021
Merged

Port Footer to NextJS #1526

merged 2 commits into from Jan 18, 2021

Conversation

HyperTHD
Copy link
Contributor

@HyperTHD HyperTHD commented Dec 12, 2020

Issue This PR Addresses

Fixes #1454

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
  • Next: Porting to NextJS

Description

This PR is a part of the transition for the telescope frontend from gatsby, to next. As part of this transition, this PR addresses the porting of the Footer component to NextJS.

Expected Output:
Output

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)

@@ -0,0 +1,207 @@
import React from 'react';
Copy link
Contributor

Choose a reason for hiding this comment

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

With next.js you don't need to import React

@HyperTHD
Copy link
Contributor Author

So 9a73f4f contains just the footer code. As that component is a standalone component compared to the AboutFooter and Banner component, I'd like to consider maybe having the other two in it's own PR just so we can have all standalone components merged. Leave your thoughts

@@ -1,4 +1,5 @@
import Head from 'next/head';
import Footer from '../components/Footer/index';
Copy link
Contributor

Choose a reason for hiding this comment

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

Change this to import Footer from '../components/Footer'; and the styling should be applied properly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed, thanks!

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.

It looks like this is now just about the Footer. If that's true, can we update the title of this?

netlify.toml Outdated


[build.environment]
API_URL="https://dev.telescope.cdot.systems"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this change in your PR?

@@ -0,0 +1,3 @@
import Footer from './Footer';
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 .ts not a .tsx file.

@@ -4,7 +4,7 @@
"private": true,
"scripts": {
"dev": "next dev -p 8000",
"build": "next build",
"build": "next build && next export",
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to modify the build command for the frontend? I think this might break something, or at least delay deployment times due to the export. Let me know!

@HyperTHD HyperTHD changed the title Port Footer, AboutFooter, and Banner to NextJS Port Footer to NextJS Jan 18, 2021
@humphd
Copy link
Contributor

humphd commented Jan 18, 2021

It looks like Netlify is unhappy:

3:08:09 PM: Deploy directory 'src/frontend/next/out' does not exist
3:08:09 PM: Failing build: Failed to build site

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.

We should sort out what is up with Netlify, but the Footer itself looks good.

Copy link
Contributor

@chrispinkney chrispinkney left a comment

Choose a reason for hiding this comment

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

Nice!

@humphd humphd merged commit ffc330a into Seneca-CDOT:master Jan 18, 2021
@HyperTHD HyperTHD deleted the issue-1454 branch February 1, 2021 13:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Next] Port Footer to NextJS
4 participants