Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Open
smark-1 wants to merge 12 commits into mkalioby:main
base: main
Choose a base branch
Loading
from smark-1:main

Conversation

Copy link

@smark-1 smark-1 commented Nov 2, 2023

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.

Copy link

@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.

Copy link
Author

smark-1 commented Sep 3, 2024

@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?

Copy link
Owner

mkalioby commented Sep 3, 2024

Ok, but we need to mention that the user shall put passkeys at the end of INSTALLED_APPS, so the template can be replaced.

Copy link

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.

Copy link
Author

smark-1 commented Sep 3, 2024

@mkalioby the setup instructions already specify the need to include passkeys in the installed apps

Copy link

rafaelurben commented Sep 3, 2024
edited
Loading

@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.

Copy link
Author

smark-1 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.

Copy link

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.

Copy link

I have now mentioned it in the README in #32.

Copy link
Author

smark-1 commented Sep 4, 2024

@mkalioby ready for review

Copy link

@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.

Copy link
Author

smark-1 commented Sep 5, 2024

@rafaelurben I don’t see any of your review comments

Copy link

@rafaelurben rafaelurben left a 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.

Copy link

@rafaelurben I don’t see any of your review comments

Oh, you're right. I'm sorry. I forgot to submit the review... 🙈

smark-1 and rafaelurben reacted with laugh emoji

Copy link
Owner

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..

Copy link
Author

smark-1 commented Sep 27, 2024

The release notes should mention that jquery is no longer a dependency and the template names are now lowercase.

Copy link

@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.

Copy link
Owner

mkalioby commented Sep 27, 2024
edited
Loading

@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.

Copy link

@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.

Copy link

@mkalioby It seems that my concerns regarding JavaScript files have been addressed in #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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@rafaelurben rafaelurben rafaelurben approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /