chore(rebuild): populate page titles #2040

montogeek · 2018-04-14T16:33:54Z

No description provided.

EugeneHlushko · 2018-04-14T17:07:00Z

src/server.jsx

      <html>
        <head>
-          <meta charSet="UTF-8" />
+          <meta charset='utf-8' />


these should be doublequotes

I agree. And in general, we should avoid misc formatting changes like line 26, lines 51 - 53, etc. I understand some of my stylistic choices might be a little different but I'd rather us fix this with something like prettier instead of changing things back and forth due to personal preference. Also, these little things create a lot of noise on PR like this, meaning it's hard to tell what's related to the title population and what are just misc formatting things.

@skipjack Yeah, another prettier change, submit a PR to add it to the repo. Sorry for the noise.

EugeneHlushko · 2018-04-14T17:08:07Z

It works locally but i have an issue with title containing empty html comment notation, if you do yarn run build and check html files in ./dist/**/*.html titles will look like this: <title>webpack | webpack</title> any idea why @montogeek

montogeek · 2018-04-14T17:11:50Z

@EugeneHlushko Nope, debugged it for 1 hour when I did it, no idea why does it happen. My best guess is that React think there is an space there, but not sure.

montogeek · 2018-04-14T17:12:35Z

webpack.prod.js

@@ -7,6 +7,17 @@ const SSGPlugin = require('static-site-generator-webpack-plugin');

 // Load Common Configuration
 const common = require('./webpack.common.js');
+const Content = require('./src/_content.json');
+
+const paths = Content.children.reduce((paths, page) => {


This can also be done in the server file, but I think it is better here.

montogeek · 2018-04-14T17:13:51Z

webpack.prod.js

@@ -27,7 +38,8 @@ module.exports = env => [
    },
    plugins: [
      new SSGPlugin({
-        crawl: true,
+        paths: Object.keys(paths),


@skipjack I really tried to use crawl option, but it fails to crawl entire configuration content, because that URL is missing from the homepage.

On the readme of static-site-generator-webpack-plugin, they say that you can use a combination of crawl: true and providing multiple entry points for crawling in the paths:

plugins: [ new StaticSiteGeneratorPlugin({ crawl: true, paths: [ '/', '/uncrawlable-page/' ] }) ]

i didnt try that but maybe you tried and it doesnt work? Just curious

@montogeek tbh I'm a little confused why the config changes were needed just to add the page titles. Did you try just importing the Content.json file and looking for the corresponding page title from within there based on locals.path? I thought the title addition would be smaller than this (although I might be missing something).

This feedback is just from a glance though, I'll look through your changes a bit more to understand exactly what you did.

Yes, that config change shouldn't be part of this PR.
I was a side change when navigating the site testing titles changed between pages.

skipjack · 2018-04-14T18:05:38Z

webpack.prod.js

@@ -7,6 +7,17 @@ const SSGPlugin = require('static-site-generator-webpack-plugin');

 // Load Common Configuration
 const common = require('./webpack.common.js');
+const Content = require('./src/_content.json');


I'm starting to understand the approach now. You can't do it this way because _content.json only exists after a build has occurred, meaning if you haven't run a build, the file won't exist and therefore this line will fail. You should take a similar approach, but do it all within the server.jsx file.

This way you won't have to change any configuration or touch any other parts of code. Then, if you remove the misc formatting things, this should be a pretty isolated, mergable PR (at least in my opinion).

i think we could also .gitignore the _content.json? why is it in repo

It is .gitignored. Everything starting with an underscore is.

Just double-checked, yeah it doesn't exist in source control (at least not on the main rebuild branch):

https://github.com/webpack/webpack.js.org/tree/rebuild/src

true, my gitignore plugin was off, thank you for doublechecking

Also, there may be a better approach for handling that content from DirectoryTreePlugin point of view. If there's a more standard approach for this kind of thing I'm all ears. Feel free to create an issue over there if necessary.

@skipjack I suspected as much, will move to server.jsx file

i think moving this part:

const paths = Content.children.reduce((paths, page) => { if (page.type === 'directory') { page.children.forEach(child => (paths[child.url] = child.title)); } else { paths[page.url] = page.title; } return paths; }, {}); ... let title = paths[locals.path];

To server JSX is actually a good idea, also i just tried

new SSGPlugin({ crawl: true, paths: [ '/', '/configuration/' ], .... })

and it seems to work, but for some reason it also crawls outgoing urls from backers

montogeek · 2018-04-14T18:48:17Z

@skipjack Addressed your comments

skipjack

Aside from the minor comment I think this is good to go. I'll test to make sure as soon as I get a chance.

On a sidenote, I think the directory-tree-webpack-plugin can provide some utilities for walking the tree (similar to how unified provides tree walking utils). This would allow us to extract many of the utility methods (e.g. in Site.jsx) outside of this repo to take the maintenance/testing burden off this site.

cc @jeremenichelli this type of abstraction is exactly what I think we've discussed to keep this repo as content focused as possible.

skipjack · 2018-04-15T03:47:19Z

DISCUSSION.md

@@ -14,7 +14,7 @@ this branch:
 - [ ] Finish re-incorporating mobile sidebar
 - [ ] Re-integrate google-analytics
 - [ ] Re-incorporate `redirects.json`
- [ ] Populate page title in `server.jsx` (fernando)
+- [x] Populate page title in `server.jsx` (fernando)


Drop this to resolve the conflict (see comments on other other PRs for context).

jeremenichelli · 2018-04-15T22:13:13Z

src/server.jsx

          <meta name="theme-color" content="#2B3A42" />
          <meta name="viewport" content="width=device-width, initial-scale=1" />
-          <title>{/* TODO */} | webpack</title>
+          <title>{title} | webpack</title>


Shouldn't we add a check for index.md? If I'm not wrong the result will be: webpack | webpack, maybe removing the title there and defaulting to webpack when it's not present.

jeremenichelli · 2018-04-15T22:15:16Z

src/server.jsx

+  }
+
+  return paths;
+}, {});


Does it makes sense to generate a _paths.json file after _content.json is created? We could import it and avoid doing this every time the server file runs, it might improve performance on SSR considering we have lots of pages.

If this is the case Fernando, we can leave it for further optimization and I can tackle that once this is merged :)

I think better than another file would just be to have a walk or find utility to find the page node within the main tree (rather than creating a separate array and walking it).

Good, to make it clear this is not a blocker for the PR to be merged.

Yeah, I think once we've tackled everything I may try to spend a day or two just doing some cleanup and extraction of code to other packages so that it's fairly clean before going into master.

In #2108, I added a content-utils.js file that should make things like this a breeze using the WalkContent or FindInContent method. I think we should get #2108 and #2074 merged into rebuild and then really start a push to finish up the rest of these. From what I've seen, most of the rebuild PRs are pretty close and may just need a little refactoring and abstracting to a separate package (e.g. remark plugins). Skimming over them again now.

montogeek · 2018-04-17T20:32:05Z

@jeremenichelli Addressed your comment, and also fixed weird characters issue: <title>webpack | webpack</title>

skipjack · 2018-04-19T07:23:36Z

@montogeek rebased and tweaked the logic to account for some undefined | webpack titles (e.g. on routes like /vote). However, I just realized something else...

This PR fixes the HTML layer titles but once the site is entered we need to dynamically change the title using something like react-helmet or react-document-title. We should keep thinking about other options though to remove this duplication. The first step would be extracting the title generation logic to a utility so it can be used both in Site.jsx and server.jsx. I can either merge this and add a TODO for SPA titles or we can hold off the merge until that final change is made.

cc @jeremenichelli @EugeneHlushko

montogeek · 2018-04-19T07:47:05Z

It is better to do it here now, I will work on it later today

skipjack · 2018-05-06T23:31:09Z

@montogeek I'm gonna wrap this one up. With #2074 in and the content utilities some of the code here can be simplified and adding react-document-title should be fairly easy (for populating page titles during SPA routing). Trying to finish this and get it merged tonight.

- Created a simple hash object to save paths and page titles - Use previous object for SSGPlugin since crawl wasn't working, it fails to collect all links ignoring 'configuration' section entirely.

…undefined` titles

Once the application is entered it acts as an SPA. This means we need to dynamically change the title here on top of the `server.jsx` static title population.

skipjack requested review from skipjack, jeremenichelli and EugeneHlushko April 14, 2018 16:40

EugeneHlushko reviewed Apr 14, 2018

View reviewed changes

montogeek commented Apr 14, 2018

View reviewed changes

skipjack reviewed Apr 14, 2018

View reviewed changes

skipjack changed the title ~~Rebuild populate page title server~~ chore(rebuild): populate page titles Apr 15, 2018

skipjack added the Infrastructure label Apr 15, 2018

skipjack mentioned this pull request Apr 15, 2018

Rebuild the Site #2039

Merged

16 tasks

skipjack requested changes Apr 15, 2018

View reviewed changes

jeremenichelli reviewed Apr 15, 2018

View reviewed changes

jeremenichelli approved these changes Apr 17, 2018

View reviewed changes

skipjack approved these changes Apr 19, 2018

View reviewed changes

skipjack force-pushed the rebuild-populate-page-title-server branch from 3298b94 to f035ea1 Compare April 19, 2018 07:15

montogeek and others added 5 commits May 6, 2018 19:36

chore(site): Add page title to server render

5160ea7

- Created a simple hash object to save paths and page titles - Use previous object for SSGPlugin since crawl wasn't working, it fails to collect all links ignoring 'configuration' section entirely.

chore(site): Simplify paths generation

7b754f9

chore(site) Move page title generation to server.jsx

75c3572

fix(site) Add exception to homepage title

18fd324

chore(rebuild): add todo and update title generation to account for `…

4ae8a04

…undefined` titles

skipjack force-pushed the rebuild-populate-page-title-server branch from 0b1aac3 to 4ae8a04 Compare May 6, 2018 23:36

skipjack added 2 commits May 6, 2018 23:02

refactor: abstract and simplify title retrieval

6cba951

chore(rebuild): add client-side title changing

ceb9411

Once the application is entered it acts as an SPA. This means we need to dynamically change the title here on top of the `server.jsx` static title population.

skipjack merged commit c22bfbd into rebuild May 7, 2018

skipjack deleted the rebuild-populate-page-title-server branch May 7, 2018 03:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(rebuild): populate page titles #2040

chore(rebuild): populate page titles #2040

montogeek commented Apr 14, 2018

EugeneHlushko Apr 14, 2018

skipjack Apr 14, 2018

montogeek Apr 14, 2018

EugeneHlushko commented Apr 14, 2018

montogeek commented Apr 14, 2018

montogeek Apr 14, 2018

montogeek Apr 14, 2018

EugeneHlushko Apr 14, 2018

skipjack Apr 14, 2018

montogeek Apr 14, 2018

skipjack Apr 14, 2018

skipjack Apr 14, 2018

EugeneHlushko Apr 14, 2018

skipjack Apr 14, 2018

skipjack Apr 14, 2018

EugeneHlushko Apr 14, 2018

skipjack Apr 14, 2018 •

edited

Loading

montogeek Apr 14, 2018

EugeneHlushko Apr 14, 2018

montogeek commented Apr 14, 2018

skipjack left a comment •

edited

Loading

skipjack Apr 15, 2018

jeremenichelli Apr 15, 2018

jeremenichelli Apr 15, 2018

jeremenichelli Apr 15, 2018

skipjack Apr 15, 2018

jeremenichelli Apr 16, 2018

skipjack Apr 17, 2018

skipjack May 5, 2018

montogeek commented Apr 17, 2018

skipjack commented Apr 19, 2018

montogeek commented Apr 19, 2018

skipjack commented May 6, 2018

chore(rebuild): populate page titles #2040

chore(rebuild): populate page titles #2040

Conversation

montogeek commented Apr 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EugeneHlushko commented Apr 14, 2018

montogeek commented Apr 14, 2018

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

skipjack Apr 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

montogeek commented Apr 14, 2018

skipjack left a comment • edited Loading

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

montogeek commented Apr 17, 2018

skipjack commented Apr 19, 2018

montogeek commented Apr 19, 2018

skipjack commented May 6, 2018

skipjack Apr 14, 2018 •

edited

Loading

skipjack left a comment •

edited

Loading