-
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
[WIP][www] Sidebars à la reactjs.org (accordions, scrollspy) #6153
Conversation
…this time against master instead of v1 ✨
# Conflicts: # www/src/templates/template-docs-markdown.js
Deploy preview for gatsbygram ready! Built with commit 6769c19 |
Deploy preview for using-drupal ready! Built with commit 6769c19 |
@@ -4,7 +4,7 @@ const path = require(`path`) | |||
const parseFilepath = require(`parse-filepath`) | |||
const fs = require(`fs-extra`) | |||
const slash = require(`slash`) | |||
const slugify = require(`limax`) |
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.
btw, is this intentional?
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 ReactJS implementation uses slugify
to construct URLs on client side. Initially I copypasta'd that and switched to slugify because limax is quite a bit heavier.
Then I changed the implementation to directly use the yaml
s href
value, so we don't need slugify
on the client anymore; left slugify
in there because for us they both work the same essentially.
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! ;-))
width: rhythm(12), | ||
}, | ||
[presets.Hd]: { | ||
width: rhythm(14), |
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.
can we get rid of this setting? It feels really wide here on my 15" screen -- and window isn't even fully expanded
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 great! Only question is my previous one: It's easy see how we could expand the Docs section to add in new Docs sections. How will this work on the tutorial section? Main question is -- how will subsequent tutorials be discoverable, since everything we put after the main tutorial series will be so far below the fold? Collapse everything by default? Nest everything under a Introduction to Gatsby section that is opened by default on the /tutorial route, that can be fully collapsed with one click? Etc. |
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 great!
Looking good! Preview URL: https://deploy-preview-6153--gatsbyjs.netlify.com/ I changed a Bound Action Creators to Actions. Some thoughts / questions:
Edit: my commit dismissed @calcsam's review, you should consider this still approved by him ✅. |
D'oh, thanks for catching that! And I was so proud for catching the changed link and Tutorial Part 8! ;-)
👍 Yeah for sure! We also want that for desktop, same issue there with the stuff further down.
Yeah, also wondered about that. Also I think we either need to throw in a |
# Conflicts: # www/src/components/sidebar-body.js
@fk 👍 The selectors used by Algolia are configured over here https://github.com/algolia/docsearch-configs/blob/master/configs/gatsbyjs.json#L36. |
* measure banner/navigation offsetHeight instead of hardcoding offset pixel value (WiP, doesn’t take non-fixed navigation on smaller screens into account yet) * right-aligned, calmer chevron * add visual separator for expanded section * bring „Tutorial“ bullets in line with design * increase sidebar inner whitespace * bring back SectionTitle <h3> (Algolia; IIRC this is semantically incorrect -> WiP)
@m-allanson aceb421 brings back I just chatted about this PR with @calcsam a few minutes ago. We decided to merge this as-is and follow up with issues tracking the remaining work. |
Can I get another review @calcsam? 🙏 😉 |
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.
👍
let { isActive } = this.props | ||
if (section.disableAccordions) { | ||
isActive = true | ||
} |
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.
don't modify props -- do something like:
const isActive = this.props.isActive || section.disableAccordions
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 dang! 😁 Babysteppin'…
Did a quick scan and looks good! |
Making „Awesome Gatsby“ a subitem was just to test functionality.
Finally getting through to answer this, sorry for taking so long @calcsam. I'd split the current/main Tutorial contents into collapsible parts/groups and automatically expand the first—essentially what you suggest in your last sentence. |
Awesome, thanks for the explanation @fk ! |
…this time against master instead of v1 (ref. #6076 and fk#2).