django-passkeys
django-passkeys copied to clipboard
mkalioby
Moved passkeys templates to passkeys subdirectory
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.
@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.
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
passkeysat 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.
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.
@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
Oh, you're right. I'm sorry. I forgot to submit the review... 🙈
Hello Guys,
Sorry for the delay.
version 2.0b1 is available now on PyPi and the code is under this branch v2.0b1
Please take the new version for a spin and let us know if it is good, so it gets released.
Welcome to the contributors..
The release notes should mention that jquery is no longer a dependency and the template names are now lowercase.
@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.
@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.
@mkalioby It seems that my concerns regarding JavaScript files have been addressed in https://github.com/mkalioby/django-passkeys/pull/48 - it might make sense to include this as part of v2.
Other than that, I have tested 2.0b1 and it looks very good so far!