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

feat: support for custom templates #129

Merged
merged 7 commits into from
Nov 10, 2023
Merged

feat: support for custom templates #129

merged 7 commits into from
Nov 10, 2023

Conversation

hacdias
Copy link
Contributor

@hacdias hacdias commented Nov 7, 2023

Closes #125 (see for more context). This PR:

  • adds -templates flag that can be used to provide a directory with templates. They override existing ones for users that want to customize how the pages look.
  • added a template for 404 pages
  • compressed template handling into serveHTML

@hacdias
Copy link
Contributor Author

hacdias commented Nov 8, 2023

@abhinav just pinging you, in case you have a chance to look at this 😄 Also, thanks for merging the other PRs!

Copy link
Collaborator

@abhinav abhinav left a comment

Choose a reason for hiding this comment

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

Thanks for this and the other PRs, @hacdias, and thanks for waiting. I had to make some time to look over this. (The other PRs were a lot quicker to review. 😅)

I'm in favor of this change in general, and I appreciate the rigor that went into this. This is a good PR. I have only a couple minor suggestions. It should be good to merge after that.

Thanks!

handler.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
handler.go Outdated Show resolved Hide resolved
handler_test.go Show resolved Hide resolved
handler_test.go Outdated Show resolved Hide resolved
@hacdias hacdias requested a review from abhinav November 9, 2023 08:20
@hacdias
Copy link
Contributor Author

hacdias commented Nov 9, 2023

@abhinav thanks! I addressed your feedback, and also cleaned up a few things I saw along the way. Looking forward to see this merged. I found many packages like this one, but none did exactly what I wanted.

Copy link
Collaborator

@abhinav abhinav left a comment

Choose a reason for hiding this comment

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

No blockers. This is great! Thanks @hacdias!

I'm okay with this as-is, but if you'd like to make getTestTemplates take a testing.TB, I can wait a day.

config_test.go Show resolved Hide resolved
handler.go Show resolved Hide resolved
handler.go Show resolved Hide resolved
handler_test.go Outdated Show resolved Hide resolved
handler_test.go Show resolved Hide resolved
handler_test.go Outdated Show resolved Hide resolved
handler_test.go Outdated Show resolved Hide resolved
handler_test.go Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
utils_test.go Outdated Show resolved Hide resolved
@hacdias
Copy link
Contributor Author

hacdias commented Nov 10, 2023

@abhinav thanks for the review! I applied the feedback, and I think it's ready to be merged.

Copy link

codecov bot commented Nov 10, 2023

Codecov Report

Merging #129 (acbf6db) into master (a068bd4) will decrease coverage by 4.04%.
The diff coverage is 65.71%.

@@            Coverage Diff             @@
##           master     #129      +/-   ##
==========================================
- Coverage   80.14%   76.11%   -4.04%     
==========================================
  Files           3        3              
  Lines         136      180      +44     
==========================================
+ Hits          109      137      +28     
- Misses         23       39      +16     
  Partials        4        4              
Files Coverage Δ
handler.go 94.91% <86.95%> (+1.16%) ⬆️
main.go 16.21% <25.00%> (+16.21%) ⬆️

📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

@abhinav abhinav merged commit bcddd3b into uber-go:master Nov 10, 2023
5 of 6 checks passed
@hacdias hacdias deleted the custom-templates branch November 10, 2023 13:23
@abhinav
Copy link
Collaborator

abhinav commented Nov 10, 2023

Merged! Thanks, @hacdias

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.

Custom Templates
2 participants