Skip to content

refactor(theme/book): Split into modules #556

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

Merged
merged 1 commit into from
Jan 21, 2018
Merged

refactor(theme/book): Split into modules #556

merged 1 commit into from
Jan 21, 2018

Conversation

sorin-davidoi
Copy link
Contributor

@sorin-davidoi sorin-davidoi commented Jan 18, 2018

Advantages:

  • Easier to reason about
  • Can easily disable some modules when debugging
  • Shared dependencies are explicit (playpen_text)
  • Enables some fancier things later one (e.g. run codeSnippets slightly later, to avoid blocking the page)

I'm aware that codeSnippets should be split into the highlighter and the editor, but I'm not sure I understand exactly how they interact so I've left it as it is for now.

Also replaced some occurences of let with var (for consistency).

Closes #559.

@projektir
Copy link
Contributor

projektir commented Jan 19, 2018

This looks great! Ran this on RBE, not seeing issues so far...

Also replaced some occurences of let with var (for consistency).

This is a bit confusing (maybe even alarming) to me. let and var do different things, there's no need to make them consistent, and, if anything, let should be preferred... changing this could, in theory, change functionality, by making something previously only available to a block available in broader scope.

@sorin-davidoi
Copy link
Contributor Author

sorin-davidoi commented Jan 19, 2018

There was no need for them to be let in the first place, since they are constants. Should we make the switch them the other way around - only use const and let? Scrap that, it's out of the scope of this PR. Will leave them as they are.

@projektir
Copy link
Contributor

Yes, we can fix all that up in a different PR. LGTM, would probably be best if another person also pulls this down and checks for issues, though.

@Havvy
Copy link
Contributor

Havvy commented Jan 20, 2018

All of the empty lines have an extra space on them.

@Michael-F-Bryan
Copy link
Contributor

@sorin-davidoi, #557 is fixed now so if you rebase onto master all the CI checks should pass.

This looks like it's ready to merge, although you'll probably want to clean up the whitespace issue @Havvy mentioned first.

Advantages:
 - Easier to reason about
 - Can easily disable some modules when debugging
 - Shared dependencies are explicit (playpen_text)
 - Enables some fancier things later one (e.g. run `codeSnippets` slightly later, to avoid blocking the page)

I'm aware that `codeSnippets` should be split into the highlighter and the editor, but I'm not sure I understand exactly how they interact so I've left it as it is for now.
@Michael-F-Bryan Michael-F-Bryan merged commit 05e4157 into rust-lang:master Jan 21, 2018
Ruin0x11 pushed a commit to Ruin0x11/mdBook that referenced this pull request Aug 30, 2020
Advantages:
 - Easier to reason about
 - Can easily disable some modules when debugging
 - Shared dependencies are explicit (playpen_text)
 - Enables some fancier things later one (e.g. run `codeSnippets` slightly later, to avoid blocking the page)

I'm aware that `codeSnippets` should be split into the highlighter and the editor, but I'm not sure I understand exactly how they interact so I've left it as it is for now.
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.

4 participants