Skip to content
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

Fix appendix section titles #6

Merged
merged 1 commit into from
Feb 6, 2023
Merged

Fix appendix section titles #6

merged 1 commit into from
Feb 6, 2023

Conversation

dlej
Copy link
Contributor

@dlej dlej commented Oct 17, 2022

The example style file has appendix titles formatted as "Appendix A.", but the current .sty file only outputs "A." by default. This can't be fixed by packages like appendix or titlesec due to the custom sectioning of the .sty file. This patch modifies the section display when the \appendix command is run so that the appendix titles are displayed correctly.

@dlej dlej changed the title Fix appendix section titles (fix #3) Fix appendix section titles Oct 17, 2022
@dlej
Copy link
Contributor Author

dlej commented Oct 17, 2022

This should the PDF referencing/table of contents issue in #3 as well, which was only resolved by removing the appendix title formatting.

@fabianp
Copy link
Member

fabianp commented Nov 1, 2022

Thanks @dlej ! Do you mind submitting also the updated sample.pdf file? Thanks!

@akucukelbir : are you OK with this change?

The example style file has appendix titles formatted as "Appendix A.", but the current `.sty` file only outputs "A." by default. This can't be fixed by packages like `appendix` or `titlesec` due to the custom sectioning of the `.sty` file. This patch modifies the section display when the `\appendix` command is run so that the appendix titles are displayed correctly.
@dlej
Copy link
Contributor Author

dlej commented Nov 7, 2022

@fabianp I have updated the sample.pdf file now. I also realized that \NewCommandCopy I had before was only supported by newer versions of LaTeX, so I replaced it with \let so that it works in older environments.

@fabianp
Copy link
Member

fabianp commented Nov 8, 2022

Thanks. Looks good to me, but I'd like to have the LGTM of our production editor @akucukelbir before merging

@akucukelbir
Copy link
Contributor

hi folks, sorry for the delay.

@dlej looks good at a quick glance! thank you!

@fabianp could you let me merge this when i have some time to update the production app? i need to keep both synced so i need a bit of time to coordinate both.

@fabianp
Copy link
Member

fabianp commented Nov 10, 2022

@akucukelbir: sure!

@fabianp
Copy link
Member

fabianp commented Jan 4, 2023

hey @akucukelbir . Happy new year and gentle ping :-)

@fabianp
Copy link
Member

fabianp commented Feb 4, 2023

hey @akucukelbir , any chance you could take a look into this? thanks!

@akucukelbir
Copy link
Contributor

Sorry for the delay. I'll merge and remove the period at the end of the Appendix. This is helpful. Thank you both.

@akucukelbir akucukelbir merged commit 117719a into JmlrOrg:master Feb 6, 2023
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.

None yet

3 participants