Refactoring Site Creation and Additions #157

LaRuaNa · 2019-03-01T14:49:15Z

Hi all,

happily we improved our codebase in last few days quite a lot! Though all the problems I run into while working on bugs led me to prepare this big refactoring PR. Please take a look and let me know if there is any room for improvements or if you have any concerns about new structure.

PS: Sorry about big PR I needed to approve my approach in most of the cases. After having all the code together I didn’t split them up again. :|

Closes:
#23
#106
#24

LaRuaNa · 2019-03-01T14:56:46Z

Hey @ollelauribostrom since this PR touches a lot of places of your first refactoring attempt would be lovely if you can get around to look over :]

ollelauribostrom · 2019-03-01T14:59:46Z

/gcbrun

ollelauribostrom · 2019-03-01T15:46:58Z

@LaRuaNa At a first glance, this looks really awesome 🎉 Will look over it more closely when I have some free time tomorrow 🙂

sagirk

Amazing job, @LaRuaNa! LGTM! 🎉

A few observations below.

gatsby-node.js

src/components/navigation-section.tsx

ahmadawais · 2019-03-01T20:41:26Z

/gcbrun

ahmadawais · 2019-03-01T20:43:59Z

All of this looks really good. Just one concern. This is a HUGE PR which I generally discourage. Lots going on in there and many decisions. Do you think we can split it?

MylesBorins · 2019-03-01T21:55:04Z

https://storage.googleapis.com/staging.nodejs.dev/60a0d17/index.html

ahmadawais · 2019-03-01T21:56:35Z

@MylesBorins doesn't seem to be working on my end. The build I mean. Want to test it.

MylesBorins · 2019-03-01T22:08:57Z

@ahmadawais there is something wrong with the build. The resources uploaded but there is no index.html

Perhaps a bug from the refactor? Can you test locally

LaRuaNa · 2019-03-02T18:15:14Z

@MylesBorins Seems to be a problem with redirects I'm on it.

ollelauribostrom

Really nice job, this is much needed and make stuff a lot more solid 👍 A few minor stuff pointed out below. You could run yarn run tslint -p ./tsconfig.json to find some minor nits (some pointed out by @sagirk below).

ollelauribostrom · 2019-03-02T20:31:56Z

src/pages/404.tsx


  return (
    <Layout title={title} description={description}>
      <Hero title={title} />
-      <Navigation sections={navigationSections} />


This removes the navigation from the 404 page, I don't think that's what we want? 🙂

I'd take care of that after refactoring the navigation if we wanna keep this. (see https://github.com/nodejs/nodejs.dev/issues/169) I don't see FWIW to run a graphql request for 404 page and additionally we would need to implement same logic we have in gatsby-nodeto provide the navigation data.

Navigation felt like a good idea on the 404 page.

+1 to retaining the navigation.

The 404 page content being boxed into the navigation area looks weird design-wise on larger screens (i.e., too much whitespace on the right).

This is caused due to the layout grid. As a hacky stopgap measure, adding an empty div in place of <Navigation> fixes it

- <Navigation sections={navigationSections} /> + <div />

On the other hand, if we don't keep the navigation, we need to tweak the text accordingly.

- The page you're trying to access does not exist. Go back to the Homepage or find what you're looking for in the menu. + The page you're trying to access does not exist. Go back to the Homepage.

However, this PR has been running for long. As suggested by @LaRuaNa, we can take care of these concerns on a follow-up PR.

test/components/pagination.test.tsx

test/components/author-link.test.tsx

src/components/navigation.tsx

src/components/article.tsx

ollelauribostrom · 2019-03-02T21:25:30Z

Not sure if this is the intended behaviour but the navigation items in the first section are no longer marked as done when selecting an item in the second section. Looks like this:

sagirk · 2019-03-03T05:00:38Z

Seems to be a problem with redirects I'm on it.

Could it be because of this duplicate redirect?

LaRuaNa · 2019-03-03T08:46:51Z

The problem with redirect is that gcp doesn't care about redirects we've in our config. So we've 2 possible ways to solve that problem:

we add redirects to firebase.json
or we copy paste our logic for /index and /learn

I'd go for firebase option. Thoughts?
@MylesBorins I assume gcp cares about firebase config in PRs as well? So we can test the PR properly

LaRuaNa · 2019-03-03T09:00:48Z

Not sure if this is the intended behaviour but the navigation items in the first section are no longer marked as done when selecting an item in the second section. Looks like this:

Yep, since they're different sections it was my interpretation. Though we can ofc change it. Any concrete thoughts / suggestions?

ollelauribostrom · 2019-03-03T10:55:35Z

In my opinion, it would make sense to mark any previous section as done as well. Let's merge this with the current behaviour, we can open a separate issue to discuss this further 🙂

LaRuaNa · 2019-03-05T03:26:31Z

@MylesBorins pink :] #157 (comment)

MylesBorins · 2019-03-05T04:59:47Z

@LaRuaNa there is nothing special GCP or Firebase related for staging... it is just a CDN bucket and the output from gatsby. I've tested between this branch and master and it appears there is a regression between master and the static output of this branch... as /index.html is being creating on master and not with this branch.

Bisecting rn to see if I can narrow down which change made this happen

LaRuaNa · 2019-03-05T05:55:47Z

@MylesBorins This line generates /index.html in master. I tried to fix it with redirects so that we redirect / to /learn but it seems like redirects don’t work out of the box. So we need to create redirects on serverside. Another quite practical way to handle this problem without messing up with serverside redirects would be we generate a single page for / with the content from introduction-to-nodejs. I'll give it a try to see how that affects build.

MylesBorins · 2019-03-05T06:01:07Z

@LaRuaNa we definitely can rely on serverside behavior for redirects but that will require us to introduce more complicated infrastructure for staging. I'm personally fond of how elegant it is that we can currently direct all traffic to index.html and it "just works". If you can replicate that behavior that would be great.

FWIW, do we need to mount everything right now on /learn? It seems premature, and something we could easily do at learn.nodejs.dev in the future without or something similar. Until we have additional material we are hosting on this site do you think there is a reason to force that path?

LaRuaNa · 2019-03-05T06:49:31Z

@LaRuaNa we definitely can rely on serverside behavior for redirects but that will require us to introduce more complicated infrastructure for staging. I'm personally fond of how elegant it is that we can currently direct all traffic to index.html and it "just works". If you can replicate that behavior that would be great.

@MylesBorins Totally agree with you on elegant infrastructure point. That's reasonable. I'll change the PR a bit so that it "just works" with staging. :] I wasn't aware of the opportunity i just mentioned.

FWIW, do we need to mount everything right now on /learn? It seems premature, and something we could easily do at learn.nodejs.dev in the future without or something similar. Until we have additional material we are hosting on this site do you think there is a reason to force that path?

Quite frankly, that's a good point I didn't question yet. /learn it's just a prefix we use rn and can be easily removed. I'll change that as well to see how it impacts our codebase, shouldn't be a big deal. Just to make it clear that isn't what causes staging problem we have.

ahmadawais · 2019-03-05T08:13:31Z

I agree with @MylesBorins assessment on the learn/ slug. Removing it would make things easier for staging infra to work. I vote for removing it altogether. Adding it later in the form of a redirect or subdomain won't be an issue.

LaRuaNa · 2019-03-05T08:14:26Z

PTAL @ollelauribostrom @sagirk

@MylesBorins Can you please check staging

ollelauribostrom

LGTM, lets get this merged and we can adress some of the above discussions later on. Would be great if you could run yarn run tslint -p ./tsconfig.json and fix a few lint issues before merging this 🙂

LaRuaNa · 2019-03-05T10:29:29Z

Argh! I messed up my tslint config while working on prettier integration. That explains why they slipped my attention :]

MylesBorins · 2019-03-05T14:45:07Z

So one thing to consider here I wasn't thinking about last night. If we get rid of the learn slug we will be breaking any links people have made to the site. Perhaps we don't care... But we should be intentional about that

…

On Tue, Mar 5, 2019, 5:29 AM Onur Laru ***@***.***> wrote: Argh! I messed up my tslint config while working on prettier integration. That explains why they slipped my attention :] — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#157 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAecVzOKvqXClcz6S8J3Mu8Yw0uNCWZyks5vTkcJgaJpZM4bZKri> .

ahmadawais · 2019-03-05T16:01:35Z

@MylesBorins we can handle that with a redirect file, not sure how it works with GCP, but for Netlify it's just a simple netlify.toml file with old to new link mapping with 301 status. Without breaking any links.

LaRuaNa · 2019-03-05T16:08:55Z

@MylesBorins I googled before I removed the rewrites. There are 2-3 links shared with /learn route. If that's really all, I think that's a bearable pain if they break. But ofc I can also rewrite /learn to / to have fallback.

sagirk

Looks good, @LaRuaNa! 🎉

A few observations below (mostly nits; fixes are tiny, but no worries if we want to take care of these on a follow-up PR).

sagirk · 2019-03-06T06:36:42Z

src/pages/404.tsx


  return (
    <Layout title={title} description={description}>
      <Hero title={title} />
-      <Navigation sections={navigationSections} />


+1 to retaining the navigation.

The 404 page content being boxed into the navigation area looks weird design-wise on larger screens (i.e., too much whitespace on the right).

This is caused due to the layout grid. As a hacky stopgap measure, adding an empty div in place of <Navigation> fixes it

- <Navigation sections={navigationSections} /> + <div />

On the other hand, if we don't keep the navigation, we need to tweak the text accordingly.

- The page you're trying to access does not exist. Go back to the Homepage or find what you're looking for in the menu. + The page you're trying to access does not exist. Go back to the Homepage.

However, this PR has been running for long. As suggested by @LaRuaNa, we can take care of these concerns on a follow-up PR.

sagirk · 2019-03-06T06:54:37Z

gatsby-node.js

+            parent: { relativePath },
+          } = node;
+
+          let previousNodeData = undefined;


nit: Can we initialize it with null instead of undefined. That way, the intent is more explicit — assigning a deliberate non-value.

let previousNodeData = null; //null - deliberate non-value

Or if we need it to be initialized with undefined, we can just leave it uninitialized.

let previousNodeData; //undefined - unintentional value

+same throughout.

test/components/pagination.test.tsx

sagirk · 2019-03-07T13:45:36Z

This PR has been open for a while now. And, #172, #168, #166 and #97 are, in fact, waiting on it to be landed.

Since it looks good for the most part, I'm merging it in its current state. We can spin up #157 (comment), #157 (comment) and #157 (comment) into new issues later.

sagirk · 2019-03-07T14:30:02Z

Since it looks good for the most part, I'm merging it in its current state. We can spin up #157 (comment), #157 (comment) and #157 (comment) into new issues later.

#157 (comment): Issue https://github.com/nodejs/nodejs.dev/issues/176
#157 (comment): Issue https://github.com/nodejs/nodejs.dev/issues/177
#157 (comment): PR #178

LaRuaNa requested a review from ollelauribostrom March 1, 2019 14:52

sagirk approved these changes Mar 1, 2019

View reviewed changes

gatsby-node.js Outdated Show resolved Hide resolved

gatsby-node.js Outdated Show resolved Hide resolved

gatsby-node.js Outdated Show resolved Hide resolved

src/components/navigation-section.tsx Outdated Show resolved Hide resolved

ollelauribostrom reviewed Mar 2, 2019

View reviewed changes

sagirk mentioned this pull request Mar 3, 2019

style: Adjust Header's padding below 380px #162

Merged

LaRuaNa mentioned this pull request Mar 3, 2019

closes #165: fix for navigation on 404 page #166

Closed

Onur Laru added 6 commits March 5, 2019 09:06

chore: Authors and section fields added.

1fdc16a

fix: Style fix for edit-link.

96e0f6f

chore: Author link component added.

c5e3356

refactor: Site creation moved to gatsby-config, nav refactored.

69ce7bd

fix: Mistakes in docs revoked.

33c407d

chore: Remove comments.

d9f4307

Onur Laru added 5 commits March 5, 2019 09:06

fix: Authorlink wont be rendered without author data.

1fd74b5

chore: Tests updated, tests for new components added.

b6881ac

chore: Remove /learn path.

193ea11

fix: Coding style fixes.

c41980f

chore: Tests updated.

9b0e891

LaRuaNa force-pushed the refactor-site-creation branch from e34f476 to 9b0e891 Compare March 5, 2019 08:08

LaRuaNa requested review from sagirk and ollelauribostrom March 5, 2019 08:12

ollelauribostrom approved these changes Mar 5, 2019

View reviewed changes

fix: Linter issues fixed

9548e20

sagirk mentioned this pull request Mar 6, 2019

Create monitoring-node-js-overview.md nodejs/website-redesign#91

Closed

sagirk approved these changes Mar 6, 2019

View reviewed changes

sagirk mentioned this pull request Mar 6, 2019

chore,refactor: Upgrade to Emotion 10; adapt in header & pagination #172

Merged

ollelauribostrom mentioned this pull request Mar 6, 2019

refactor: Use IntersectionObserver in Layout component #168

Merged

sagirk merged commit e7574b8 into nodejs:master Mar 7, 2019

This was referenced Mar 7, 2019

chore: Run prettier/tslint in ci #97

Merged

fix: Initialize with null to assign a deliberate non-value #178

Merged

Refactoring Site Creation and Additions #157

Refactoring Site Creation and Additions #157

Conversation

LaRuaNa commented Mar 1, 2019

LaRuaNa commented Mar 1, 2019

ollelauribostrom commented Mar 1, 2019

ollelauribostrom commented Mar 1, 2019

sagirk left a comment

Choose a reason for hiding this comment

ahmadawais commented Mar 1, 2019

ahmadawais commented Mar 1, 2019

MylesBorins commented Mar 1, 2019

ahmadawais commented Mar 1, 2019

MylesBorins commented Mar 1, 2019

LaRuaNa commented Mar 2, 2019

ollelauribostrom left a comment

Choose a reason for hiding this comment

ollelauribostrom Mar 2, 2019

Choose a reason for hiding this comment

LaRuaNa Mar 5, 2019

Choose a reason for hiding this comment

ahmadawais Mar 5, 2019

Choose a reason for hiding this comment

sagirk Mar 6, 2019

Choose a reason for hiding this comment

ollelauribostrom commented Mar 2, 2019 • edited Loading

sagirk commented Mar 3, 2019

LaRuaNa commented Mar 3, 2019

LaRuaNa commented Mar 3, 2019

ollelauribostrom commented Mar 3, 2019

LaRuaNa commented Mar 5, 2019

MylesBorins commented Mar 5, 2019

LaRuaNa commented Mar 5, 2019

MylesBorins commented Mar 5, 2019 • edited Loading

LaRuaNa commented Mar 5, 2019

ahmadawais commented Mar 5, 2019

LaRuaNa commented Mar 5, 2019

ollelauribostrom left a comment

Choose a reason for hiding this comment

LaRuaNa commented Mar 5, 2019

MylesBorins commented Mar 5, 2019 via email

ahmadawais commented Mar 5, 2019

LaRuaNa commented Mar 5, 2019

sagirk left a comment

Choose a reason for hiding this comment

sagirk Mar 6, 2019

Choose a reason for hiding this comment

sagirk Mar 6, 2019

Choose a reason for hiding this comment

sagirk commented Mar 7, 2019

sagirk commented Mar 7, 2019

ollelauribostrom commented Mar 2, 2019 •

edited

Loading

MylesBorins commented Mar 5, 2019 •

edited

Loading