-
-
Notifications
You must be signed in to change notification settings - Fork 93
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
Switch to new "bit explanation" format #334
Conversation
23df373
to
d7d175f
Compare
I'm not going to go over the MMM01 page, because it's too daunting even for me. |
I've converted almost everything: 3 SGB pages are remaining, and the MMM01 page. Plus anything I've missed, but honestly converting these is getting too tedious even for my tastes, and I'd rather get all of the existing conversions merged instead of making this PR linger in limbo any longer. |
Note: a render is still available at https://eldred.fr/pandocs. Please refer to the PR contents for a list of modified tables. |
Recovered from a very old stash, hopefully I "rebased" correctly. Since this is changing the structure of the bit explanations, some modifications to the content have been made as well.
This guarantees good X overflow behaviour. Nice!
@@ -130,6 +130,51 @@ Note that the angle brackets [are only required if there are spaces in the URL]( | |||
|
|||
In effect, this means that linking to a section is as simple as copy-pasting its name in the URL field, prepending a `#`, and wrapping everything in `<>` if the name contains a space. | |||
|
|||
### Bit breakdown tables |
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.
Are we sure this goes in DEPLOY.md and not CONTRIBUTING?
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 all of the custom syntax is documented here: better keep it all together now, and move it in a separate PR?
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.
Agree!
@@ -35,24 +40,75 @@ impl Preprocessor for Pandocs { | |||
} | |||
|
|||
fn run(&self, _: &PreprocessorContext, mut book: Book) -> Result<Book, 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.
Could we add some comments/documentation on what these new parts/functions of the preprocessor are doing?
@@ -35,24 +40,75 @@ impl Preprocessor for Pandocs { | |||
} | |||
|
|||
fn run(&self, _: &PreprocessorContext, mut book: Book) -> Result<Book, 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.
Also, shouldn't we add some simple unit testing to this logic?
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'm not sure what you'd want to unit-test here, and not super hot on trying to build a mdBook book from scratch for testing purposes—to what extent is that stable? I'd like to avoid churn there.
increasing: bool, | ||
} | ||
|
||
impl<'input> BitDescrAttrs<'input> { |
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.
Shouldn't this be tested as well?
Honestly, thank you. |
As discussed in gbdev#334 (comment)
As discussed in gbdev#334 (comment)
As discussed in gbdev#334 (comment)
Fixes #318, #293
TODO:
CGB palette
in the OAM table)