[docs update] fix tabs on 'installation' page #10593
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
**what is the change?:** This is an awful hack. It will be improved in the future. - Fork `<MarkdownPage>` to create a custom page for Installation, to support the tab panel interaction - Take the raw JS from `installation` doc file and put it in the new `<InstallationPage>` - Change the `block` tags into `section` tags, and remove dead JS from `installation` doc file. Because that is less gross than leaving `block` tags in place. TODO: - fix styling - make follow-up tasks and issues to properly refactor this **why make this change?:** The tabs were not working, we need to have them a least functional. **test plan:** Manual testing **issue:** Checklist item in wiki - Misc > Properly handle tabbed content on /docs/installation.html
**what is the change?:** - Downgraded some 'h2' elements to 'h3' - Tweaked JS to add a class to the parent's parent instead of just the parent of the content section **why make this change?:** - The default 'h2' styles in the new Gatsby site add extra space and an 'hr' type line at the top, and this didn't look right inside the tabbed content. - The class was supposed to go on the parent of the tabs and their content, and in a previous commit I added an extra wrapper to the content sections. That meant the JS should be updated to find the right place to add the class. **test plan:** Manual testing (flarnie will add screenshots) **issue:** https://github.com/bvaughn/react/wiki/Gatsby-check-list misc > Properly handle tabbed content on /docs/installation.html
|
|
bvaughn
suggested changes
Sep 5, 2017
www/src/templates/docs.js
Outdated
| /> | ||
| ); | ||
| const Docs = ({data, location}) => { | ||
| console.log('location is ', location); |
There was a problem hiding this comment.
:) fixed while you were reviewing
www/src/templates/docs.js
Outdated
| const Docs = ({data, location}) => { | ||
| console.log('location is ', location); | ||
| if (location.pathname === '/docs/installation.html') { | ||
| console.log('about to render InstallationPage'); |
www/src/templates/docs.js
Outdated
| ); | ||
| const Docs = ({data, location}) => { | ||
| console.log('location is ', location); | ||
| if (location.pathname === '/docs/installation.html') { |
There was a problem hiding this comment.
This works but I think we'd be better off doing this check in gatsby-node like we do for the Home page.
There was a problem hiding this comment.
Cool - will look into that.
**what is the change?:** - removed `console.log` statements - moved a variable to the correct place, fixing lints - changed a query selector so that loading a page with a hash in the url selects the right tab in the 'installation' docs **why make this change?:** - to get CI passing - to fix hash urls on the 'installation' page **test plan:** - manual testing - `yarn run lint` **issue:** Checklist item - https://github.com/bvaughn/react/wiki/Gatsby-check-list misc > Properly handle tabbed content on /docs/installation.html
**what is the change?:** Instead of adding a fork in the logic in the 'docs' template, we add a special case in the router itself. That way we branch at a place that makes sense in the code. **why make this change?:** To keep the code organized and more maintainable. **test plan:** Manual testing. **issue:** Checklist item on gatsby docs update wiki; https://github.com/bvaughn/react/wiki/Gatsby-check-list misc > Properly handle tabbed content on /docs/installation.html
bvaughn
reviewed
Sep 6, 2017
| display('target', nextTab); | ||
| } | ||
| } | ||
| window.keyToggle = keyToggle; |
There was a problem hiding this comment.
Whoops! I missed this.
We can't have window references in code that's auto-executed as part of an import b'c they break the Gatsby build step. I'll remove this one! 😄
bvaughn
reviewed
Sep 6, 2017
| defaultActiveSection={findSectionForPath( | ||
| location.pathname, | ||
| sectionList, | ||
| )} |
There was a problem hiding this comment.
Same here as with window. Should be:
defaultActiveSection={
location != null
? findSectionForPath(location.pathname, sectionList)
: null
}We also need to pull location from Gatsby rather than relying on the global.
My bad for overlooking.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
problem:
The old "installation" page relied on HTML with inline CSS and JS, and the JS doesn't work with our new site infrastructure.
Specifically, there were some tabs which switched the content and they are not working in the new docs site.
We are trying to make minimal changes to the markdown docs files, because we want to minimize merge conflicts and also not interfere with an ongoing i18n project.
solution:
Made minimal changes to the installation docs file in 'docs';
Then we made a hacky custom component and moved the tab JS there, mostly unchanged.
Ideally we do a follow-up pass and completely move the styles and JS out of 'docs/docs/installation.md' and clean this up. But for now it's similar to other work-arounds in place for this docs rewrite.