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.

Sign up for GitHub

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

Jump to bottom

ape:0.1.0 #1596

Merged
merged 11 commits into from
Jan 21, 2025
Merged

ape:0.1.0 #1596

merged 11 commits into from
Jan 21, 2025

Conversation

gris-chat
Copy link
Contributor

@gris-chat gris-chat commented Jan 19, 2025 •
edited
Loading

I am submitting

  • a new package
  • an update for a package

Description: Cf README.md

I have read and followed the submission guidelines and, in particular, I

  • selected a name that isn't the most obvious or canonical name for what the package does
  • added a typst.toml file with all required keys
  • added a README.md with documentation for my package
  • have chosen a license and added a LICENSE file or linked one in my README.md
  • tested my package locally on my system and it worked
  • excluded PDFs or README images, if any, but not the LICENSE
  • ensured that my package is licensed such that users can use and distribute the contents of its template directory without restriction, after modifying them through normal use.

@typst-package-check typst-package-check bot added the new A new package submission. label Jan 19, 2025
@elegaanz
Copy link
Member

Thanks for the submission! I have two points that I would like to suggest before merging this package:

  • first of all, you seem to use a mix of English and French to name your functions and arguments. It would be nice if you could chose a single language and stick to it (preferably English).
  • you seem to use camelCase to name your functions and arguments. The Typst standard library and most packages use kebab-case, so to be more consistent with the ecosystem it would be nice to change that. This is not a mandatory requirement, so if you have a strong preference towards camelCase, just tell me.

@gris-chat
Copy link
Contributor Author

I'll work on it, thanks

@gris-chat
Copy link
Contributor Author

I tried to follow your suggestions. Hope I edited everything.

elegaanz
elegaanz reviewed Jan 21, 2025
View reviewed changes
Copy link
Member

@elegaanz elegaanz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still a few occurrences of camelCase that were not caught :)

packages/preview/ape/0.1.0/style/outline.typ Outdated
@@ -0,0 +1,88 @@
#let getOutline(lang) = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function still uses camelCase.

packages/preview/ape/0.1.0/style/front-pages.typ Outdated
@@ -0,0 +1,151 @@
#let front-pages(title, title-page, authors, outline, customOutline) = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

customOutline should be custom-outline.

packages/preview/ape/0.1.0/tools/shortcuts.typ Outdated
// Physique
#let dt = $dif t$
#let ar = math.arrow
#let Nar(c) = math.norm(math.arrow(c))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe the capital letter is relevant here, but if it is not it should be called nar.

@gris-chat
Copy link
Contributor Author

My bad, I missed it...
I hope it's done !

@elegaanz
Copy link
Member

Looks good now. Thank you!

@elegaanz elegaanz merged commit ee2eab2 into typst:main Jan 21, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@elegaanz elegaanz elegaanz left review comments

Assignees
No one assigned
Labels
new A new package submission.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@gris-chat @elegaanz