-
Notifications
You must be signed in to change notification settings - Fork 26
Moved passkeys templates to passkeys subdirectory #27
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
Conversation
# Conflicts: # passkeys/templates/passkeys/passkeys.js
rafaelurben
commented
Sep 3, 2024
@smark-1 While you are already moving the files, I think it would make sense to make them all lowercase.
Having uppercase letters in file names can lead to hours of debugging when working with both case-sensitive and case-insensitive operating systems.
@rafaelurben Thanks for pointing this out to me. This would be a great opportunity to rename the files but it also is a much bigger breaking change. If the user is not just moving the files to a different directory, capitalization will likely be missed.
@mkalioby what do you think?
Ok, but we need to mention that the user shall put passkeys
at the end of INSTALLED_APPS, so the template can be replaced.
rafaelurben
commented
Sep 3, 2024
Thanks for pointing this out to me. This would be a great opportunity to rename the files but it also is a much bigger breaking change. If the user is not just moving the files to a different directory, capitalization will likely be missed.
I don't think that this really is a bigger change. Both ways are a breaking change (=> major version bump) and manual work is required either way. And move/rename are essentially one step, if done via command line.
There should just be a clear migration guide, which would also mention that jQuery is no longer required.
Ok, but we need to mention that the user shall put
passkeys
at the end of INSTALLED_APPS, so the template can be replaced.
This seems like a very good idea.
@mkalioby the setup instructions already specify the need to include passkeys in the installed apps
@mkalioby the setup instructions already specify the need to include passkeys in the installed apps
Yes, but not that the order matters (which might not be known to beginners).
I'll fix this in another PR, as it isn't directly related to this one.
I thought that the templates directory took priority over app directories and order only mattered within app template directories. I will have to do some further testing on this.
rafaelurben
commented
Sep 3, 2024
I thought that the templates directory took priority over app directories and order only mattered within app template directories. I will have to do some further testing on this.
That is correct. But an app might want to extend the features of django-passkeys and thus might need to override the templates.
rafaelurben
commented
Sep 3, 2024
I have now mentioned it in the README in #32.
@mkalioby ready for review
rafaelurben
commented
Sep 5, 2024
@smark-1 I have left 2 review comments on your commits with a small change I think would make sense to add to this PR.
Currently, ’python manage.py collectstatic’ would fail on newer Django versions.
@rafaelurben I don’t see any of your review comments
@rafaelurben
rafaelurben
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, ’python manage.py collectstatic’ would fail on newer Django versions. See comments for details.
rafaelurben
commented
Sep 8, 2024
@rafaelurben I don’t see any of your review comments
Oh, you're right. I'm sorry. I forgot to submit the review... 🙈
The release notes should mention that jquery is no longer a dependency and the template names are now lowercase.
rafaelurben
commented
Sep 27, 2024
@mkalioby Great news! I'll test it as soon as I can.
Is there a specific reason why my PR (#32) is not included in v2.0?
Also, I've been wondering... If file names are already being changed, would it make sense to also rename "check_passkeys.js" and "passkeys.js"? Currently, my syntax highlighter goes completely crazy as these are HTML files disguised as JS files.
I'd be happy to make this change in a PR against the v2.0 branch and also write a detailed migration guide for v2, if you think this is a good idea.
@mkalioby Great news! I'll test it as soon as I can.
Is there a specific reason why my PR (#32) is not included in v2.0?
These shall be merged as v1.2.8, then will show up on v2.0
Also, I've been wondering... If file names are already being changed, would it make sense to also rename "check_passkeys.js" and "passkeys.js"? Currently, my syntax highlighter goes completely crazy as these are HTML files disguised as JS files.
Change them to what? and these are mainly js but have django templates inside mainly for urls
I'd be happy to make this change in a PR against the v2.0 branch and also write a detailed migration guide for v2, if you think this is a good idea.
rafaelurben
commented
Sep 27, 2024
@mkalioby Thanks for the clarification! :)
I would have renamed passkeys.js
to scripts/passkey_login.html
as it is already completely valid HTML and currently completely bricks auto formatting and syntax highlighting.
I would have personally also changed check_passkeys.js
to scripts/passkey_check.html
and added a script tag around the code, even though I am aware that this would mean that the position of the include would have to be changed too.
I think it would make the code more readable. But if you think this is a bad idea, it's completely fine.
I moved all the templates to a subdirectory in the templates folder called passkeys. This is standard in most Django apps and Django docs recommends this so that it minimizes the risk of collisions.
Note: This is a breaking change, but it is a very easy fix for each project.