-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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
feat(www): Ecosystem page content - finished #9442
feat(www): Ecosystem page content - finished #9442
Conversation
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.
Couple notes and thoughts for consideration. It's looking beautiful!! 💜
www/gatsby-node.js
Outdated
@@ -9,6 +9,11 @@ const url = require(`url`) | |||
const getpkgjson = require(`get-package-json-from-github`) | |||
const parseGHUrl = require(`parse-github-url`) | |||
const { GraphQLClient } = require(`graphql-request`) | |||
const yaml = require("js-yaml") |
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.
Did you look at pulling this in using gatsby-transformer-yaml
instead? This shouldn't be 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.
<EcosystemSection | ||
title="Plugin Library" | ||
description="With 429 plugins, lorem ipsum sunt we need some proper copy here pulling in data from your favorite source is only a few mouseclicks away." | ||
subTitle="Fetured Plugins" |
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.
"Featured" (minor, just noting it while we see it!)
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.
👍
<EcosystemSection | ||
title="Starters" | ||
description="Starters are boilerplate Gatsby sites maintained by the community which give you a headstart for your Gatsby project." | ||
subTitle="Fetured Starters" |
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.
"Featured"
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.
👍
www/gatsby-node.js
Outdated
// add lists of featured items to Ecosystem page | ||
if (page.path === "/ecosystem/") { | ||
const { createPage, deletePage } = actions | ||
return new Promise(resolve => { |
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 a Promise is needed here!
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 don't believe this section will be necessary -> https://github.com/gatsbyjs/gatsby/pull/9442/files/c74e8d61c236371c553b1d634ecbca8d371544cb#diff-fb043578f52339bcb2a2d2b4df1203c8
Edit: I'm wrong
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.
@DSchau
When I remove the Promise like that
exports.onCreatePage = ({ page, actions }) => {
// add lists of featured items to Ecosystem page
if (page.path === "/ecosystem/") {
const { createPage, deletePage } = actions
const oldPage = Object.assign({}, page)
page.context.featuredStarters = ecosystemFeaturedItems.starters
page.context.featuredPlugins = ecosystemFeaturedItems.plugins
deletePage(oldPage)
createPage(page)
}
}
it works, but could you give me a hint why there is a promise in the docs
https://www.gatsbyjs.org/docs/creating-and-modifying-pages/#modifying-pages-created-by-core-or-plugins? For me it seems like a similar case.
CC: @amberleyromo
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.
@greglobinski that's a great point. To me, that example should be re-written, there's nothing async required there as far as I can tell.
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.
👍
www/src/assets/plugins-ecosystem.svg
Outdated
@@ -0,0 +1,38 @@ | |||
<svg viewBox="0 0 32 32" fill="none" xmlns="http://www.w3.org/2000/svg"> |
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.
nit: but we can/should optimize/prettify these. I've always liked https://jakearchibald.github.io/svgomg/
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.
sure
www/src/components/button.js
Outdated
@@ -34,6 +35,7 @@ const Button = ({ | |||
...(secondary && { ...buttonStyles.secondary }), | |||
...(large && { ...buttonStyles.large }), | |||
...(small && { ...buttonStyles.small }), | |||
...(tiny && { ...buttonStyles.tiny }), |
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 new here, but I'm not sure I see the value of spreading to a new object here?
...(tiny && buttonStyles.tiny)
should do the same thing, correct?
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.
👍
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.
😬 👋 🙆♂️
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.
@fk tbh i have no idea what that means
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.
@amberleyromo I think he's referring to the fact that he originally wrote it!
@fk correct if I'm wrong, haha!
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.
@fk I also :)
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.
y'all are more emoji-native than I am!
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.
Yep, that was me saying "argh, hello, i did this, OK, i get it now". And putting myself in Amberley's shoes, probably wouldn't have figured it out myself. 😅
www/src/pages/ecosystem.js
Outdated
node: { slug, name, description, humanDownloadsLast30Days }, | ||
} = item | ||
|
||
return { |
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.
Maybe just return the node, since you're grabbing every field anyways?
i.e.
const plugins = pluginsData.map(({ node }) => node)
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.
👍
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 is such clean work, @greglobinski. Thanks so much! I just have a couple of tiny requests, while we have this in PR.
import { rhythm, options } from "../../utils/typography" | ||
import presets, { colors } from "../../utils/presets" | ||
|
||
const EcosysteSectionRoot = styled("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.
Small cleanup task -- make this EcosystemSectionRoot
? (tiny misspelling)
import styled from "react-emotion" | ||
import { Link } from "gatsby" | ||
|
||
import ArrowForwardIcon from "react-icons/lib/md/arrow-forward" |
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 used
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.
we should provide some linting step or something to catch these early!
(tangential to this PR!)
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.
we should provide some linting step or something to catch these early!
That would be great 👍
import React from "react" | ||
import PropTypes from "prop-types" | ||
import styled from "react-emotion" | ||
import { Link } from "gatsby" |
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 used
import React from "react" | ||
import PropTypes from "prop-types" | ||
import styled from "react-emotion" | ||
import { Link } from "gatsby" |
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 used
@@ -1,28 +1,114 @@ | |||
import React, { Component } from "react" | |||
import Helmet from "react-helmet" | |||
import { graphql } from "gatsby" |
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.
Looks like a couple of unused imports in this file, too.
- "react-helmet"
- "../utils/typography"
- "../utils/presets"
- "../components/layout/page-heading"
@@ -4,6 +4,7 @@ const colors = { | |||
// original palette by @SachaG | |||
// @see https://www.figma.com/file/J6IYJEtdRmwJQOrcZ2DfvxDD/Gatsby | |||
gatsby: `#663399`, // was #744c9e | |||
gatsbyDark: `#442266`, |
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.
were these color additions discussed with @fk? just making sure.
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.
@amberleyromo No, sorry I forgot, I got it from Figma
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.
And did not want to add it as a hard coded inline value inside the component, but definitely @fk sould take a loot at this.
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.
👍 All fine (as expected ;-))—also not explicitly discussing those before IMO as the design defines them.
Actually I saw them a couple days already and was happy to find out I managed to cook up a design that doesn't add 10 greys ;-) …
That said, I absolutely will appreciate a heads up if I go overboard on new colors (or too light ones, I do that way too often), or any critique or suggestion on how to keep color additions down to a minimum. More generally speaking—in any case, if there's a better way to archive a similar goal, or you have any concerns about what I'm suggesting, or better ideas: Please don't hesitate to bring it up!
Also I'm even more sold on this styling technique. I found it very clean to read. @DSchau @jlengstorf |
@amberleyromo 🎉 glad to hear it! I still do think it's worth doing @greglobinski do you have any preference here? My preference is a small one, totally fine writing it either way! |
@greglobinski I'm also going to write some copy for the three sections, so we can get rid of the placeholder text 😅 |
@DSchau It's not a preference but I made a comment here about the shortcut syntax |
Edit: Compelling: #9377 (comment). Thanks @greglobinski. |
@amberleyromo please take a look at my comment here #9377 (comment) first |
🎉 💜 🤗 🙏 😅 |
* feat(www): prepare featured items forEcosystem * feat(www): add new icons for Ecosystem * feat(www): Ecosystem content * feat(www): create Ecosystem mobile styles * feat(www): add new variant of Button * feat(www): Ecosystem desktop styles * feat(www): add thumbnail to featured starters * fix(www): fix button label * fix(www): fix typo * refactor(www): prettify svg code * refactor(www): remove unnecessary destructuring * fix(www): use slug to pick up featured starters * refactor(www): remove unnecessary spreading to a new object * fix(www): fix stylelint errors * fix(www): use gatsby-transformer-yaml to pull featured Ecosystem items * feat(www): add backkground icons to test it * refactor(www): remove unnecessary destructuring in Button * fix(www): remove unnecessary infos * fix(www): remove background * feat(www): add function preventing too long description of featured items * fix(www): add border to starter thumbnails * fix(www): refine Tablet styles * fix(www): fix desktop section height on FF * fix(www): change Ecosystem sections headers * fix(www): remove unused imports
* feat(www): prepare featured items forEcosystem * feat(www): add new icons for Ecosystem * feat(www): Ecosystem content * feat(www): create Ecosystem mobile styles * feat(www): add new variant of Button * feat(www): Ecosystem desktop styles * feat(www): add thumbnail to featured starters * fix(www): fix button label * fix(www): fix typo * refactor(www): prettify svg code * refactor(www): remove unnecessary destructuring * fix(www): use slug to pick up featured starters * refactor(www): remove unnecessary spreading to a new object * fix(www): fix stylelint errors * fix(www): use gatsby-transformer-yaml to pull featured Ecosystem items * feat(www): add backkground icons to test it * refactor(www): remove unnecessary destructuring in Button * fix(www): remove unnecessary infos * fix(www): remove background * feat(www): add function preventing too long description of featured items * fix(www): add border to starter thumbnails * fix(www): refine Tablet styles * fix(www): fix desktop section height on FF * fix(www): change Ecosystem sections headers * fix(www): remove unused imports
www/src/data/ecosystem/featured-items.yaml
file with manually selected plugins and starters featured on Ecosystem pageonCreatePage
ingatsby-node.js
js-yaml
to loadfeatured-items.yaml
ecosystem-icons.js
helpertiny
variant ofButton
componentecosystem-board.js
,ecosystem-section.js
,ecosystem-featured-items.js
,ecosystem-featured-item.js
pages/ecosystem.js
I'm not sure if the way I implemented 'manually' featured items of starters and plugins for the page is the right way to do it in the end, but for development it works. Please give me directions if it needs to be implemented in other way.
It's WIP, there are, first of all, responsiveness issues which needs to be resolved, I've already asked @flo for the meeting to talk about them.