-
Notifications
You must be signed in to change notification settings - Fork 7.7k
Refactor home page #150
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
Refactor home page #150
Conversation
Deploy preview ready! Built with commit 65f1ced |
content/marketing/component-based.md
Outdated
@@ -0,0 +1,8 @@ | |||
--- | |||
title: Component-Based | |||
order: 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Used to decide the ordering of the 3-column marketing content. order
might not be the best name for this and suggestions are welcome! I briefly considered rank
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think order works for this. Kind of like flexbox order.
@@ -60,15 +60,7 @@ exports.createPages = async ({graphql, boundActionCreators}) => { | |||
allMarkdown.data.allMarkdownRemark.edges.forEach(edge => { | |||
const slug = edge.node.fields.slug; | |||
|
|||
if (slug === '/index.html') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No longer need to programatically create home page because it is now within the pages
directory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dig it.
src/pages/index.js
Outdated
@@ -6,7 +6,7 @@ | |||
|
|||
'use strict'; | |||
|
|||
import ButtonLink from './components/ButtonLink'; | |||
import ButtonLink from '../templates/components/ButtonLink'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should <ButtonLink>
be moved to src/components
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
src/pages/index.js
Outdated
border: 'none', | ||
borderBottom: `1 solid ${colors.divider}`, | ||
}} /> | ||
<section className="home-section"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will refactor the examples section in a later PR. Please bear with the hardcoding of class names for now.
src/types.js
Outdated
@@ -23,6 +23,7 @@ export type Node = { | |||
next?: string, | |||
prev?: string, | |||
title: string, | |||
order?: number, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure whether this is necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really, since the order
frontmatter field is only used in the GraphQL query (and not in any Flow-covered code).
content/marketing/component-based.md
Outdated
@@ -0,0 +1,8 @@ | |||
--- | |||
title: Component-Based |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I considered putting the title inside the content instead but I think putting it in the frontmatter makes more sense so we leave the formatting of the title outside this file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds reasonable to me.
@bvaughn Have fixed according to your comments. Do have a look again 😄 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry it took me so long to get back to this. I had less time during the work week this week to review website PRs.
I left some thoughts. Also it looks like we have a merge conflict now that needs to be addressed. Maybe you could rebase or merge master into this branch?
Let's talk more if any of my comments are unclear. The biggest remaining "TODO" really is to move the remaining hard-coded content into markdown files. Otherwise I dig this 👍
content/marketing/component-based.md
Outdated
@@ -0,0 +1,8 @@ | |||
--- | |||
title: Component-Based |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds reasonable to me.
content/marketing/component-based.md
Outdated
@@ -0,0 +1,8 @@ | |||
--- | |||
title: Component-Based | |||
order: 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think order works for this. Kind of like flexbox order.
@@ -60,15 +60,7 @@ exports.createPages = async ({graphql, boundActionCreators}) => { | |||
allMarkdown.data.allMarkdownRemark.edges.forEach(edge => { | |||
const slug = edge.node.fields.slug; | |||
|
|||
if (slug === '/index.html') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dig it.
@@ -6,7 +6,7 @@ | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works for the /
route but not for /index.html
We need to add an explicit redirect to gatsby-node.js
to handle this:
// Without this, /index.html won't redirect to root.
createRedirect({
fromPath: '/index.html',
redirectInBrowser: true,
toPath: '/',
});
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, thanks for this!
src/pages/index.js
Outdated
<div className="example"> | ||
<h3>A Simple Component</h3> | ||
<p> | ||
React components implement a `render()` method that takes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The content for these sections also needs to come from markdown files (so it can be localized).
src/pages/index.js
Outdated
JSX is optional and not required to use React. | ||
</strong>{' '} | ||
Try the{' '} | ||
<a href="http://babeljs.io/repl#?babili=false&browsers=&build=&builtIns=false&code_lz=MYGwhgzhAEASCmIQHsCy8pgOb2vAHgC7wB2AJjAErxjCEB0AwsgLYAOyJph0A3gFABIAE6ky8YQAoAlHyEj4hAK7CS0ADxkAlgDcAfAiTI-hABZaI9NsORtLJMC3gBfdQHpt-gNxDn_P_zUtIQAIgDyqPSi5BKS6oYo6Jg40A5OALwARCHwOlokmdBuegA00CzISiSEAHLI4tJeQA&debug=false&circleciRepo=&evaluate=false&lineWrap=false&presets=react&prettier=true&targets=&version=6.26.0"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You know what might be cool? To auto-generate the URL for this from the actual code example (so the two will always stay in-sync). Babel's REPL just uses the lz-string
module to serialize and deserialize code so we could copy it. 😁 It isn't likely to change because it's important to Babel to maintain backwards compatibility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this link will be part of the markdown, how do we generate the URL for this? Do we generate after render and modify the href
in the markup?
Thanks for the review @bvaughn! I fixed the Regarding the content for the examples, I intend to address and fix them in a follow up PR so that this PR does not get too big. As for the refactoring of the marketing content, I think they are good to be merged. WDYT? |
I think we should do it all as one PR, to be honest. We have localization/translation efforts going on right now and I don't want to temporarily pull a bunch of home page content out of the pipeline and cause confusion. Also cc @ericnakagawa about this change. |
I see. Ok I'll continue on this branch then 😄 |
@bvaughn I extracted out the examples content into markdown files following the same approach for the marketing content. I intend to also move the code out into separate files but I'm not too sure how the As far as the localization/translation efforts goes, this PR should be ok given that I have extracted out both the marketing and examples content into markdown files. |
If this link will be part of the markdown, how do we generate the URL for this? Do we generate after render and modify the href in the markup? |
I dig the general direction this PR is heading 😁 Thanks for driving this! Could we move the new directories
Maybe we should ignore the TODO about moving the hard-coded examples, for now, at least pending the outcome of #138 and #162. What I was thinking was that we could move the example JS snippets near the markdown, but if we did that we wouldn't be able to import/require them in the template without either (a) passing them through via markdown (kind of hacky) or (b) overriding the Webpack config to allow it to resolve paths within Maybe for now, we should just rename the code snippet constant names to better match the names of their corresponding markdown files.
It's fine to do this as a follow-up. Actually I'd be happy to do it b'c it sounds fun. 😄 The idea though is that we could come up with a new syntax for linking to an example file and then we could maybe write a custom Remark transform plugin that generates the actual link (with lz compression) from it during build time so they were guaranteed to stay in sync. |
Friendly follow up with @yangshun 😄 Also cc @ericnakagawa |
One last friendly follow up 😄 If I don't hear back in a day or so, I'm going to take over this PR and implement my own requested changes. (This is totally okay, of course.) |
@bvaughn Very sorry for the delay! This PR slipped my mind. I shifted the files into |
No worries at all. Let me take a look. |
Thank you! I've added to this PR by moving the example JavaScript code into |
This PR is part one of the refactoring of the home page issue #97 . I didn't want the PR to become too big so I left out refactoring of the examples section. I wanted to get some opinions on the current approach before starting on the examples section as well.
This PR does a few things:
templates/home.js
topages/index.js
content/index.md
intopages/index.js
I have removed most of the hardcoded class names. There are some leftover styles because I have yet to refactor the examples section. I intend to refactor that part in a similar fashion and extract out the text into Markdown files and also load them via GraphQL. Will follow up with another PR for the examples section later.
cc @bvaughn