-
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
gatsby features page #2443
gatsby features page #2443
Conversation
Deploy preview failed. Built with commit 4d8bc1b https://app.netlify.com/sites/using-glamor/deploys/59e2c6fadf995311d14d18fd |
Deploy preview ready! Built with commit 4d8bc1b |
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-browser.js
Outdated
@@ -0,0 +1,9 @@ | |||
exports.onRouteUpdate = (location) => { | |||
const hash = location.hash || (location.location && location.location.hash) |
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 already setup in gatsby-link — try debugging that to see why it's not working?
gatsby/packages/gatsby-link/src/index.js
Line 126 in 22e8755
element.scrollIntoView() |
@@ -16,6 +16,7 @@ export default ({ children, className, hasSideBar = true }) => ( | |||
[presets.Tablet]: { | |||
paddingBottom: rhythm(1.5), | |||
}, | |||
...css, |
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 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`, |
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.
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}> |
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.
oh maybe this is why clicking the links aren't scrolling? I think just using <Link>
here would solve it.
This is great! 👍 Two things I'd like to add to Kyle's thoughts (disclaimer: I'm 🤒🤕):
Happy to help! |
Cool 👍 , doing a couple rounds of fixes incorporating this feedback and then will send back |
@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 ) |
@KyleAMathews okay we're good for another look on this. Page is now responsive and margins and alignments behave reasonably. |
Sweet! Tons of improvement since last time I looked. A few thoughts:
I notice a few copy tweaks I want to suggest. Monday morning I'll print out the page and ink it up with suggestions. |
Though if you're itching to set it live, the copy editing can wait until after it's deployed. |
Cool, I'll add the bottom border back in and equally space the columns. The highlighting on 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. |
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. |
@KyleAMathews -- okay, got the hover effect working, added the bottom border back, equally spaced the columns. We're good to go. |
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. |
Done 👍 |
🎉 Fantastic addition to the website! |
@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 ? |
Would love a PR fixing things! |
As reported/suggested by @tjgilpin in gatsbyjs#2443 (comment)
As reported/suggested by @tjgilpin in #2443 (comment)
Add v0 of the new features page to Gatsby website. Preview at https://oceanographer-panda-78512.netlify.com/features/