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

Reduce overhead of upgrades for users with custom stylesheets/JS #3051

Merged
merged 5 commits into from
Dec 3, 2017
Merged

Reduce overhead of upgrades for users with custom stylesheets/JS #3051

merged 5 commits into from
Dec 3, 2017

Conversation

techknowlogick
Copy link
Member

I have custom JS/CSS in use on my gitea instance and when I upgrade I have to edit head.tmpl and footer.tmpl to add in my changes again (otherwise I might have out of date templates). This will allow me to put all of my changes in header_extras.tmpl/footer_extras.tmpl and not worry about upgrading.

Open to suggestions regarding the names of the additional templates, or their locations.

@codecov-io
Copy link

codecov-io commented Dec 1, 2017

Codecov Report

Merging #3051 into master will decrease coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3051      +/-   ##
==========================================
- Coverage   33.43%   33.41%   -0.03%     
==========================================
  Files         270      270              
  Lines       39553    39553              
==========================================
- Hits        13226    13216      -10     
- Misses      24441    24447       +6     
- Partials     1886     1890       +4
Impacted Files Coverage Δ
models/repo_indexer.go 45.54% <0%> (-3.47%) ⬇️
models/repo.go 37.96% <0%> (-0.19%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4947cfb...06c3ed2. Read the comment docs.

@tboerger tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Dec 1, 2017
Copy link
Member

@sapk sapk left a comment

Choose a reason for hiding this comment

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

A little change needed and add 2-3 lines (in https://github.com/go-gitea/gitea/blob/master/docs/content/doc/advanced/customizing-gitea.en-us.md) recommending to use extra folder to add some custom header and footer resources to keep template updated.

@@ -66,5 +66,6 @@
<!-- JavaScript -->
<script src="{{AppSubUrl}}/vendor/plugins/semantic/semantic.min.js"></script>
<script src="{{AppSubUrl}}/js/index.js?v={{MD5 AppVer}}"></script>
{{template "base/footer_extras" .}}
Copy link
Member

Choose a reason for hiding this comment

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

Cleaner to create a folder extra at root of templates containing footer.tmpl and header.tmpl.

@techknowlogick
Copy link
Member Author

@sapk thanks for the feedback. I've changed the directory for the extras template files, and added in some documentation on when to use them.

@sapk
Copy link
Member

sapk commented Dec 1, 2017

LGTM

@tboerger tboerger added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Dec 1, 2017
@ethantkoenig
Copy link
Member

Would custom be a better directory name than extras?

@techknowlogick
Copy link
Member Author

@ethantkoenig Thanks for feedback. As custom is already a top level dir, when we tell people to go to the custom folder it might create confusion to have multiple dirs with the same name. (See use of custom on this page: https://docs.gitea.io/en-us/customizing-gitea/ )

@ethantkoenig
Copy link
Member

@techknowlogick Could we explicitly tell them to go to templates/custom, so they don't get confused?

@techknowlogick
Copy link
Member Author

@ethantkoenig yes, but then for the other custom directory we have to explicitly say not the templates/custom directory. For example we have instructions for adding the robots.txt, and serving public files to put all of those in custom or custom/public. There is also a user on discord that is currently confused about just even the one custom directory.

However, this is as much commitment to the idea of that folder name as I have, and so in light of the above if you still want me to change the directory name I will.

@ethantkoenig
Copy link
Member

ethantkoenig commented Dec 2, 2017

I personally prefer custom over extras, for two reasons:

  1. I think it more clearly describes the intent of the directory (customized template changes). If I saw the name extras, my first instinct would be that it is a directory of miscellaneous templates, not customizable ones.
  2. For consistency with the existing custom directory; every directory named custom is for some sort of custom change.

@techknowlogick
Copy link
Member Author

techknowlogick commented Dec 2, 2017 via email

@techknowlogick
Copy link
Member Author

@ethantkoenig I've updated it. Thanks again for the feedback.

cc: @lafriks

@lafriks
Copy link
Member

lafriks commented Dec 2, 2017

LGTM

@tboerger tboerger added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Dec 2, 2017
@lafriks lafriks merged commit b0971ae into go-gitea:master Dec 3, 2017
@techknowlogick techknowlogick deleted the template_customizations branch December 3, 2017 00:29
@lunny lunny added this to the 1.4.0 milestone Dec 3, 2017
@lunny lunny added the type/enhancement An improvement of existing functionality label Dec 3, 2017
@go-gitea go-gitea locked and limited conversation to collaborators Nov 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants