-
Notifications
You must be signed in to change notification settings - Fork 25
[Enhancement] Timber Styles #127
Conversation
zsherman
commented
May 26, 2018
- Allow choice of syntax highlighter (Prism or Highlight.js)
- Add divider component to sidebar
- Update markdown styling and fix headers
- Update search styles, scroll active item into view, better empty state, add icons, detect outside click
- Add automatic table of contents generation
- Add some responsive layout logic
- Add better content loading state
Pull Request Test Coverage Report for Build 163
💛 - Coveralls |
src/core/output.js
Outdated
| await fs.outputJson(outputJson, { content: item.content }) | ||
| await fs.outputJson(outputJson, { | ||
| content: item.content, | ||
| toc: toc(item.content).json |
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'd extract this all to another function since you're using it again in src/core/socket.js.
|
|
||
| const content = `module.exports = { | ||
| theme: require('${rsh}/styles/prism/${theme_custom.syntaxTheme}').default, | ||
| theme: require('${rsh}/styles/${syntax.renderer}/${syntax.theme}').default, |
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.
Definitely don't think the user should have to specify prism or highlight. We should automatically default to one or the other depending on which one supports the language.
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 agree, but let's roll that up into the language detection epic.
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 we do default to Highlight.js now.
src/utils/config.js
Outdated
| syntaxTheme: 'prism', | ||
| syntaxLineNumbers: false, | ||
| prefixTitles: true, | ||
| tableOfContents: 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.
Either use table_of_contents, or switch up the other config options to use camel case. Should stay consistent.
themes/default/routes.js
Outdated
| render={({ staticContext }) => ( | ||
| <Page | ||
| route={staticContext || data} | ||
| route={staticContext ? JSON.parse(staticContext) : data} |
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.
Huh? This will break things. staticContext is already a JSON object, trying to parse it again will throw an error.
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.
Yeah not sure why I did that...fixed.