Skip to content
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

gatsby features page #2443

Merged
merged 10 commits into from
Oct 15, 2017
Merged

gatsby features page #2443

merged 10 commits into from
Oct 15, 2017

Conversation

calcsam
Copy link
Contributor

@calcsam calcsam commented Oct 13, 2017

Add v0 of the new features page to Gatsby website. Preview at https://oceanographer-panda-78512.netlify.com/features/

@KyleAMathews
Copy link
Contributor

KyleAMathews commented Oct 13, 2017

Deploy preview failed.

Built with commit 4d8bc1b

https://app.netlify.com/sites/using-glamor/deploys/59e2c6fadf995311d14d18fd

@gatsbybot
Copy link
Collaborator

gatsbybot commented Oct 13, 2017

Deploy preview ready!

Built with commit 4d8bc1b

https://deploy-preview-2443--gatsbygram.netlify.com

Copy link
Contributor

@KyleAMathews KyleAMathews left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good!

A few design bits of feedback

features-eval-1

features-eval-2

@@ -0,0 +1,9 @@
exports.onRouteUpdate = (location) => {
const hash = location.hash || (location.location && location.location.hash)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is already setup in gatsby-link — try debugging that to see why it's not working?

element.scrollIntoView()

@@ -16,6 +16,7 @@ export default ({ children, className, hasSideBar = true }) => (
[presets.Tablet]: {
paddingBottom: rhythm(1.5),
},
...css,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't necessary as the Glamor plugin we add converts the css prop into a className before passing it in which is already being added to the div.

return `none`
}
const basicStyling = {
width: `30px`,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please replace all pixel values with usages of rhythm. This helps keep spacing consistent across the site.

</Link>
{
section.links[title][0] == `#` ?
<a href={section.links[title]} css={linkStyle}>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh maybe this is why clicking the links aren't scrolling? I think just using <Link> here would solve it.

@KyleAMathews
Copy link
Contributor

@fk
Copy link
Contributor

fk commented Oct 13, 2017

This is great! 👍

Two things I'd like to add to Kyle's thoughts (disclaimer: I'm 🤒🤕):

  1. The table content currently runs wider than the layout container: bildschirmfoto 2017-10-13 um 18 18 45
    While the aforementioned is remedied relatively easy – we could for example win some horizontal space by turning the "Category" column into its own row – we also need to make the tables behave on mobile/small screens: bildschirmfoto 2017-10-13 um 18 21 19
    An easy way is to scroll the whole table like Bootstrap does: https://getbootstrap.com/docs/4.0/content/tables/#responsive-tables … and then there's
  1. I think we should indicate that the "Feature" links are no regular links but toggle additional information. Adding a caret or info icon would do IMO?

Happy to help!

@calcsam
Copy link
Contributor Author

calcsam commented Oct 13, 2017

Cool 👍 , doing a couple rounds of fixes incorporating this feedback and then will send back

@calcsam
Copy link
Contributor Author

calcsam commented Oct 14, 2017

@fk -- really helpful thoughts on making the content responsive, ended up going with a similar solution to the ones you suggested. Still needs a few tweaks but almost good to go. (cc @KyleAMathews )

@calcsam
Copy link
Contributor Author

calcsam commented Oct 14, 2017

@KyleAMathews okay we're good for another look on this. Page is now responsive and margins and alignments behave reasonably.

@KyleAMathews
Copy link
Contributor

Sweet! Tons of improvement since last time I looked. A few thoughts:

  • I miss the lines in between rows. They blend a bit too much now visually. Why not just keep the default table styles there? You can see a table at the bottom of https://www.gatsbyjs.org/packages/gatsby-image/
  • highlighting of links is weird in a few places e.g. "Component ecosystem"
  • Spacing of columns varies quite a bit. Those should be equal

I notice a few copy tweaks I want to suggest. Monday morning I'll print out the page and ink it up with suggestions.

@KyleAMathews
Copy link
Contributor

Though if you're itching to set it live, the copy editing can wait until after it's deployed.

@calcsam
Copy link
Contributor Author

calcsam commented Oct 14, 2017

Cool, I'll add the bottom border back in and equally space the columns.

The highlighting on componentization is due to how the :hover selector on <a> plays with break-word seems unlikely that we can do a lot about that (and it's the only current example)

image

I'm up for merging in when these are addressed and then editing on Monday and following. Probably best to ship first and then keep improving afterwards.

@KyleAMathews
Copy link
Contributor

I dunno that the hover effect is necessary here so perhaps it'd be better to just kill it rather than have it look silly sometimes.

@calcsam
Copy link
Contributor Author

calcsam commented Oct 14, 2017

@KyleAMathews -- okay, got the hover effect working, added the bottom border back, equally spaced the columns. We're good to go.

@KyleAMathews
Copy link
Contributor

Looking great! One last visual tweak as I was scrolling through would be to reduce the font-size of labels on each row as they'd fit better smaller and also for info hierarchy that'd be better as they'd be smaller than the font at the top of each section.

@calcsam
Copy link
Contributor Author

calcsam commented Oct 15, 2017

Done 👍

@KyleAMathews
Copy link
Contributor

🎉

Fantastic addition to the website!

:shipit:

@KyleAMathews KyleAMathews merged commit 209afca into gatsbyjs:master Oct 15, 2017
@tjgilpin
Copy link

tjgilpin commented Oct 18, 2017

@KyleAMathews @calcsam - Noticed a typo in the last sentence of the second list item for CMSs (Wordpress) in the Features section

Depending onYou can self-host your website, or use an official hosting provider.

should probably read something like

Depending on your requirements you can self-host your website, or use an official hosting provider ?

@KyleAMathews
Copy link
Contributor

Would love a PR fixing things!

fk added a commit to fk/gatsby that referenced this pull request Oct 19, 2017
@fk fk mentioned this pull request Oct 19, 2017
KyleAMathews pushed a commit that referenced this pull request Oct 19, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants