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

fileserver: Fix browse name_dir_first sorting #4218

Merged
merged 2 commits into from
Jul 7, 2021

Conversation

diamondburned
Copy link
Contributor

This pull request fixes browse's default template to make name_dir_first sorting work.

The fix is basically a minor typo fix, changing namedirfirst in the template to name_dir_first to reflect sortByNameDirFirst = "name_dir_first".

@francislavoie
Copy link
Member

I think the better fix would be to change the sortByNameDirFirst variable's value to namedirfirst, because some users will have copied the template to customize it. Changing the variable would also fix it for those users, whereas changing the template in Caddy would only fix it for users who didn't copy the template.

@francislavoie francislavoie added the bug 🐞 Something isn't working label Jun 21, 2021
@francislavoie francislavoie added this to the v2.4.4 milestone Jun 21, 2021
@francislavoie francislavoie requested a review from mholt June 21, 2021 09:03
@francislavoie
Copy link
Member

@diamondburned Do you have time to address my comment? Thanks!

This commit fixes the `sortByNameDirFirst` variable inside fileserver to
match what browse's default template has.
@diamondburned
Copy link
Contributor Author

@diamondburned Do you have time to address my comment? Thanks!

I've made this change, though I still have some second thoughts about it, mostly because the browse template that I wrote ended up using name_dir_first for the form, so existing templates would likely have to make the fix once this is merged.

@francislavoie
Copy link
Member

Well you're the first person whose reported it, so I have high doubts anyone else would have fixed it in the same way you did.

I'm not so concerned for people who may have fixed it in their own apps and not reported the issue, because that's not being a good user of open source software.

I'm more concerned about users who copied the template and changed other things but left that the same, which will be the vast majority of people who copied templates in total. Like probably everyone except you in that group of people.

Thanks for reporting and it for the fix! 😀

@francislavoie francislavoie merged commit 9e16e80 into caddyserver:master Jul 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐞 Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants