Switch to Next.js #178

mattxwang · 2021-06-18T19:42:13Z

This PR does the meat of our transition to Next.js. As a heads up, the files changed/diff is absolutely nasty; I imagine that there will be little bits and pieces that we'll have to address down the line.

Summary

We transition the site to be Next SSG'd.

CRA-to-Next transition

swaps out toolchain to use Next
- installs next, and relevant eslint plugins
- removes react-scripts, react-router-dom, react-router-hash-link, and @testing-library equivalents
- changes npm scripts to use next defaults
- swaps out node-sass for sass (the reference dart implementation)
uses Next's file-based routing
- currently segments into six pages: about, committee, events, index (previously Home), sponsors, and techgala; no paths are broken
- moves the rest of components (including GM) into components
restructures SASS and CSS
- moves all styles into styles folder, mirroring pages and components structure
- collates a few styles
- imports Font Awesome into the next bundle, instead of loading into the index.html entrypoint
uses next/image when possible
- in some cases, we import the image out of the public folder; this automatically sets width and height
- in some other cases, manually sets width and height (typically for UI elements) or uses layout="fill" and retools CSS to properly deal with it
- in some cases, maintains <img />. we were unable to fully transition over to <Image /> (see below conversation)
for ease-of-transition, removes all tests (jest, enzyme, related things)

Things that will need to be addressed later

this PR explicitly doesn't address .module SASS, but we should do that in a follow-up
~~there should be something we can do about fonts that doesn't involve loading it in the <Head> - let's look into that!~~ possibly see @BryanPan342's comment below
SEO, using <Head> properly, etc; since this isn't a regression, let's table adding this to each page in a separate PR
more ergonomic image inputs - maybe through an index.js that collates together images, thanks @dtjanaka for the suggestion!
remove postcss@8 as a dev dependency, but still resolve this error: missing postcss dependency on build vercel/next.js#24882
refactor the GM component to derive all of its information from state; then, make a new wrapper page that passes in the relevant information from data files, manual configuration, etc.
document the new CI/CD pipeline, document Next quirks (ex next/image)
resolve issues with next/image

Misc. code quality changes

removes the <Button> component entirely, and changes the button selector to instead use the .button class.
- this is because the component itself didn't do anything (it just passed on props to a <button>) and also arbitrarily restricted the semantic element
changes CSS for the officers section of the about page to use flexbox instead of nested selectors + absolute/inline-based positioning
changes the officers section text to say "Leadership" <- this is easily changeable
fixes bug where .page-content p bleeds into the Committees page events arbitrarily
fixes incorrect use of ul and li in <Navbar>
slashes a non-trivial amount of completely unused CSS
de-dupes CommitteeSectionIntro's double-images for committee motifs

Issues that still need to be addressed:

This PR also does some favours for us: it closes #162, (reasonably) closes #131 (for now). It also unblocks #110 and #116!

…ng; known issues surroudning images and FA5

BryanPan342 · 2021-06-19T07:22:33Z

@mattxwang lmk if u need help with the next.js migration!

Even tho next is awesome, sometimes the bugs are opaque AF and takes a bit of digging to fix!

…images poorly on committees page

mattxwang · 2021-06-20T03:16:07Z

@mattxwang lmk if u need help with the next.js migration!

Even tho next is awesome, sometimes the bugs are opaque AF and takes a bit of digging to fix!

Thanks, appreciate it! I might pick your brain once I resolve all the low hanging fruit. Gotta say, the image conversion has been the largest pain point thus far, though it's also a huge code smell with our website in general!

BryanPan342 · 2021-06-20T06:33:11Z

For fonts, usually we import them into our variables or global scss file!

https://github.com/UCLA-Creative-Labs/sunshine/blob/master/styles/_variables.module.scss

BryanPan342

Why are u using document? What part of the html/body tag do you really need to target?

pages/_document.js

Resolved TODO Converted from div with background image to next image component

Issues: image gets center aligned and styling is inline will fix this soon

advaithg

I fixed the issues with the weird sizing of Next Images by making the attributes of the Image element layout: "fill" with objectFit:"contain" and wrapping them in a div which had the size we needed along with position:relative

This made sense cause the images on the website mostly had a fixed height but varying width, and Next didn't let me do that - so I had to do that in an outer div. I couldn't find a way to tell Next to "use default width"

Alignment was done using the objectPosition attribute

done by adding distDir in next.config.js

BryanPan342 · 2021-06-21T15:46:44Z

Never got back to this; I'm curious, does this delay the font load time? Since then we'll have to hit the CSS file first (fetch + parse), and then load the font itself (fetch + parse)?

it does a little bit (I only really notice it when I'm really laggy), but I mean, it will load with the rest of your styling so I don't see a problem with it TBH

BryanPan342 · 2021-06-21T15:48:17Z

@BryanPan342, I'm curious do y'all host CL stuff on Vercel or Netlify? Running into some issues with using next/image on Netlify (you can peep the deploy preview or check out the discussion vercel/next.js#18356 or vercel/next.js#19065 for some context). No worries at all on timing though, you're doing a huge help for us already!

we host everything on netlify! I haven't run into many problems with next/image BUT to be honest, I prefer using just the regular image tag! I find next/image abstracts a little too much for me personally.

have you tried removing the next.config file? I found that that file usually gives me the most trouble

this netlify post seems to confirm a lot of your suspicions.. yeah honestly would recommend just using <image /> and also I'm not sure if you need the next.config file.. i honestly don't know what craCompact is but I deleted your next.config file and it still deploys

mattxwang · 2021-06-22T02:46:36Z

we host everything on netlify! I haven't run into many problems with next/image BUT to be honest, I prefer using just the regular image tag! I find next/image abstracts a little too much for me personally.

Hm okay thanks for the thought - I think we'll switch back to <img> for now (since it moves gracefully), and try our hand at <Image /> some other time (I also saw that there's some movement with the Essential Next.js plugin).

For some SVG's, converted to PNG For others, reverted back to img instead of using /next/image reverts acm logo on navbar to img reverts committee spread to img also comments out next image import runs linter reverts sidebar in committees page to img reverts committee logos on committee page to img fixed bbq picture on about page converts svgs in sponsors page to pngs revert bbq picture to img fixes linter error

(combination of 3 commits - messages below) added borders to committee page images also added margin before intro section for gap after committee logo issue currently - 'padding' between image and it's surrounding div causing the border to be slightly off fixed border issue for committees images also added comment explaining rationale behind gnarly css ran linter

Got a bit lazy :/

dtjanaka

nice

dtjanaka · 2021-06-26T01:20:14Z

components/GM.js

@@ -0,0 +1,177 @@
+// import React from 'react';


Why is this file completely commented out?

I think GM needs to be refactored into an actual component in a dummy page, will probably be another issue!
@mattxwang should have more info

Yeah, just to follow up - since the page isn't live, didn't want to introduce more complexity into the change. Eventually, we should adjust this too.

A quick thought - I think the page for a GM (ex gm-w21) should wrap the existing GM component, and simply pass in props to the component. That way, the GM component itself doesn't need to rely on external data sources, which may be a better separation of concerns.

All the above comments make sense.

Since we're now relying on file system routing, it's harder to have dummy pages that are not live. Matt's suggestion makes a lot of sense, having the actual pages acting just as wrapper for components (to which you can pass all the relevant data as props).

dtjanaka · 2021-06-26T01:23:59Z

components/TechGala/ProjectCard.js

 import React from 'react';

 function ProjectCard(props) {
    const {image, title, winner, summary, names, url, category} = props.project;
    return (
        <div className="intro-row">
            <div className="show-mobile">
-                <img src={image} alt={`showcase image for ${title}`}/>
+                <Image src={image} alt={`showcase image for ${title}`} height={280} width={370} />


Maybe a dumb question, but it seems like height and width are now required props for images; what are the implications for responsiveness?

The images automatically readjust themselves based on the size of the page, iirc. The height and width props are for the general case.

Yeah, I think moving forward @advaithg is going to be the SME for next/image - you may want to discuss how the component works (ex specified with/height versus layout="fill", what's happening under the hood) with the team some time.

dtjanaka · 2021-06-26T01:27:14Z

data/sponsors.js

@@ -0,0 +1,69 @@
+import blizzard from '../public/images/sponsors/blizzard.png';


maybe consider making an index.js file for the images so u can just do import {blizzard, citadel, deloitee, etc.} from '../public/images/sponsors/';

Great idea, let's add it to the backlog 😄

evanzhong

Amazing work @advaithg and @mattxwang!

I mainly looked at the diffs in this review. Most of the relevant items are left in comments in specific files. General things I noticed

+1 to the comment in the PR writeup, there's a lot of low hanging fruit with using .modules; a discussion we should have later after this PR ships.
There's some extraneous commented out JSX left in files under TODOs comments. My personal preference is not to have commented out code in production. If we can always refer back to it in the commit history if needed down the line. I left some comments about this, but didn't comment on every instance I saw.
We use both regular html <img> tags and next/image. This could get confusing in future development as devs might not know which is appropriate. I know next/image was a particular pain point so we can talk about addressing this in a future issue.

Only thing blocking for me is removal of commented out code, other than that it looks good to go!

README.md

evanzhong · 2021-06-28T17:49:01Z

components/Carousel.js

-							<div
-							style={{backgroundImage: 'url('+ process.env.PUBLIC_URL + item + ')'}}>
-							</div>
+							<Image src={item} width={IMAGE_WIDTH} height={IMAGE_WIDTH}/>


I remember there being some discussion about using regular <img> tags instead of next/image.

This also seems to be a background image that we're replacing, would appreciate some more context behind this change

Yeah, so my reasoning here is that the background image approach itself is a hack - each div really semantically should be an image, and without this change (in general) the Carousel would be inaccessible.

I did merge that above change with also transitioning it to <Image />, which may have jumped the gun. Didn't see any issues with this on prod, but always down to switch back!

Makes sense, re: accessibility.

If no problems seen on prod with next/image I think it's actually a great change. The carousel is a prime place for lazy loading since it's located below the fold and is image dense

components/CommitteeSpread.js

evanzhong · 2021-06-28T17:51:22Z

components/Committees/CommitteeEventCard.js

 import React from 'react';

 function CommitteeEventCard(props) {
    const hasImage = props.image.src;
    return (
        <div className="info-card-container">
+            <div className="image-wrapper">


Assuming that this div.image-wrapper is necessary to control the width and height of the next/image used below

I believe so, I think this is from Advaith's side!

components/Committees/CommitteeSection.js

evanzhong · 2021-06-28T18:06:38Z

components/Navbar.js

+// import Image from 'next/image';
+import Link from 'next/link';
+import React from 'react';
+// import gmData from '../../config/gmData';
+
+//to allow GM page to be visible on the navbar and accessible
+//uncomment the above import line "import gmData from '../../config/gmData';" to get quarter from the gmData page
+//uncomment the two lines saying  "/* <Link href="/gm"><li>{gmData.date.quarter} gm</li></Link> */"


As this is written right now, this is kind of a hack, but it's just a result of how we designed the Navbar component

I'm curious what y'alls thoughts are on making this more friendly to page inactivation. We could easily store the ul information in a json object and have the Navbar be constructed from it. I think this would also benefit us by making the code DRYer.

Yeah, I'm not the largest fan of this either, especially since this requires people to update comments (uncommon) when implementation details change, like they did here.

The JSON implementation is a good idea; our friends at Creative Labs do something similar, see https://github.com/UCLA-Creative-Labs/sunshine/blob/master/components/Navbar.tsx#L9

pages/events.js

pages/index.js

styles/components/CommitteeSpread.scss

evanzhong · 2021-06-28T18:21:50Z

styles/pages/committees.scss

+  // this is kind of gnarly, but the !important override is needed to overwrite the CSS built into /next/image
+  // the div is to change display from inline-block to block so there's no weird spacing between border and image
+  // the img is to overwrite the default image shadow from committees.scss to prevent double borders
+  // img should be removable once fully converting to /next/image - the one in committees.scss needs to stay for the non next imgs for now
+  div {
+    display: block !important;
+  }
+
+  img {
+    box-shadow: none !important;
+  }  


Interesting stuff, perhaps this is what Bryan mentioned about next/image being too abstracted for his liking.

In the future, if this persists we may want to consider creating our own wrapper component for next/image

Yup yup, honestly strange that they force so many style tag changes (causes some specificity shenanigans). Open to any suggestions on how to approach this.

mattxwang · 2021-06-28T18:57:09Z

Addressed most of Evan's comments, and updated the PR description with the relevant to-dos!

evanzhong

lgtm for this set of changes!

first-pass: next as a library, directory restructuring, nav and routi…

f82d326

…ng; known issues surroudning images and FA5

mattxwang marked this pull request as draft June 18, 2021 19:42

undo delete README.md

e66af16

mattxwang added 4 commits June 19, 2021 18:02

fix sponsors images

9f9ffef

remove instances of Button, fixes FA5, minor formatting

0289dba

bandaid patch officers section

b5bfd39

minimum reproducible build -> adds eslint, postcss8 as dev dep, does …

5caece6

…images poorly on committees page

mattxwang added 4 commits June 19, 2021 20:30

properly removes officers

7ac4c36

minor content fixes

e37b963

mediocre sizing attempt

b73cc5c

(mostly) resolves navbar issues

e9ee742

mattxwang added 2 commits June 19, 2021 23:35

configure eslint plugin; resolve navbar (again), use _document

28c0933

extra TODO

2ac497e

BryanPan342 reviewed Jun 20, 2021

View reviewed changes

pages/_document.js Outdated Show resolved Hide resolved

mattxwang and others added 7 commits June 19, 2021 23:55

resolves favicon path, thanks bryan!

e128d44

changes carousel image

23434e2

Resolved TODO Converted from div with background image to next image component

changed committee spread to next image

0952dd0

Issues: image gets center aligned and styling is inline will fix this soon

fixed issues with converting committee spread to next image

0b307b9

converted news articles to next image

6dc2b1e

fixed committee navbar image sizing

0856d9c

fixed membership portal button padding on mobile

47f88f3

advaithg reviewed Jun 20, 2021

View reviewed changes

advaithg and others added 5 commits June 20, 2021 22:17

ran linters

77699b9

changed build directory to build

bf95ff3

done by adding distDir in next.config.js

adds netlify.toml

6bafa24

serverless target?

6dddf24

rm index.html

dc41ddd

mattxwang mentioned this pull request Jun 21, 2021

Bump eslint from 7.28.0 to 7.29.0 #181

Merged

advaithg force-pushed the next-switch branch from 73e0a49 to 17f6635 Compare June 22, 2021 17:35

mattxwang mentioned this pull request Jun 22, 2021

Add 2020 ACM award to "In The News" section #182

Closed

advaithg added 2 commits June 23, 2021 16:33

fixes tech gala page images

1a9730f

advaithg force-pushed the next-switch branch from 7be7f20 to 68fff49 Compare June 24, 2021 09:29

mattxwang added 3 commits June 24, 2021 16:21

adds priority=true for relevant images

3526673

Footer Image -> img

99699a4

rm unused import

4b2a6db

Got a bit lazy :/

mattxwang marked this pull request as ready for review June 25, 2021 05:42

advaithg requested a review from a team June 26, 2021 00:46

dtjanaka approved these changes Jun 26, 2021

View reviewed changes

mattxwang requested a review from evanzhong June 28, 2021 15:36

This was referenced Jun 28, 2021

Bump node-sass from 6.0.0 to 6.0.1 #183

Closed

Bump @testing-library/react from 11.2.7 to 12.0.0 #184

Closed

evanzhong requested changes Jun 28, 2021

View reviewed changes

removes commented code, adds TODO messages, adds some alt text

2885085

evanzhong approved these changes Jun 28, 2021

View reviewed changes

temp alt tag for carousel

9296d2f

mattxwang merged commit 4b0d94e into main Jun 28, 2021

mattxwang deleted the next-switch branch June 28, 2021 20:52

This was referenced Jun 29, 2021

consider collating image imports #189

Closed

refactor the GM component #190

Closed

Benchmark and possibly change the way fonts are imported #193

Open

mattxwang mentioned this pull request Jun 30, 2021

Link embeds are flaky #194

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch to Next.js #178

Switch to Next.js #178

mattxwang commented Jun 18, 2021 •

edited

BryanPan342 commented Jun 19, 2021

mattxwang commented Jun 20, 2021

BryanPan342 commented Jun 20, 2021

BryanPan342 left a comment

advaithg left a comment

BryanPan342 commented Jun 21, 2021

BryanPan342 commented Jun 21, 2021 •

edited

mattxwang commented Jun 22, 2021

dtjanaka left a comment

dtjanaka Jun 26, 2021

advaithg Jun 26, 2021

mattxwang Jun 27, 2021

evanzhong Jun 28, 2021

dtjanaka Jun 26, 2021

advaithg Jun 26, 2021

mattxwang Jun 27, 2021

dtjanaka Jun 26, 2021

mattxwang Jun 27, 2021

evanzhong left a comment

evanzhong Jun 28, 2021

mattxwang Jun 28, 2021

evanzhong Jun 28, 2021

evanzhong Jun 28, 2021

mattxwang Jun 28, 2021

evanzhong Jun 28, 2021

mattxwang Jun 28, 2021

evanzhong Jun 28, 2021

mattxwang Jun 28, 2021

mattxwang commented Jun 28, 2021

evanzhong left a comment

		@@ -0,0 +1,69 @@
		import blizzard from '../public/images/sponsors/blizzard.png';

Switch to Next.js #178

Switch to Next.js #178

Conversation

mattxwang commented Jun 18, 2021 • edited

Summary

CRA-to-Next transition

Things that will need to be addressed later

Misc. code quality changes

Issues that still need to be addressed:

BryanPan342 commented Jun 19, 2021

mattxwang commented Jun 20, 2021

BryanPan342 commented Jun 20, 2021

BryanPan342 left a comment

Choose a reason for hiding this comment

advaithg left a comment

Choose a reason for hiding this comment

BryanPan342 commented Jun 21, 2021

BryanPan342 commented Jun 21, 2021 • edited

mattxwang commented Jun 22, 2021

dtjanaka 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

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

evanzhong 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

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

mattxwang commented Jun 28, 2021

evanzhong left a comment

Choose a reason for hiding this comment

mattxwang commented Jun 18, 2021 •

edited

BryanPan342 commented Jun 21, 2021 •

edited