-
Notifications
You must be signed in to change notification settings - Fork 2
Migrate to allauth #83
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
base: main
Are you sure you want to change the base?
Conversation
lisannengel
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.
lgtm :)
fluegelk
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.
Nice work overall, I left a handful of comments but nothing major:
- it would be nice to have either the user dropdown in the new settings or at least show the current user name when changing it (or both)
- I would add the license for the copied allauth templates
- for the github settings: please check if we need all scopes currently requested (or if that setting is even used). We should use the minimal scope necessary.
As already mentioned on Monday, quite a lot of the changed lines are due to reformatting (especially in the HTML files), which makes it harder to see the actual changes. I'll enable auto-formatting in the CI which should resolve at least basic reformattings, but for future PRs please also check the diff of your PR and try to only include necessary changes. If large scale reformatting of existing code is necessary, it might be better do that in a separate PR which doesn't change functionality.
fluegelk
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.
Seems like you need to apply the pre-commit hooks manually since the pre-commit.ci has no permissions to push to the other CANVAS repo.
All other comments have been addressed so it would be ready to merge on the pre-commit passes.
7403b58 to
0a4bc3e
Compare
fluegelk
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.
One final thing: some of the docstrings in darkmode.js were better before the merge from main, if possible (i.e. they are still correct) change them back to the more descriptive version.
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.
Some of the docstrings were better before these most recent changes. Unless they are no longer correct, revert to the old docstrings (as suggested below) or merge the old and new ones.
Co-authored-by: fluegelk <35153980+fluegelk@users.noreply.github.com>
Co-authored-by: fluegelk <35153980+fluegelk@users.noreply.github.com>
Co-authored-by: fluegelk <35153980+fluegelk@users.noreply.github.com>
|



Warning
[Secrets] are not passed to workflows that are triggered by a pull request from a fork.so manual execution for the test is needed here. (They run fine locally on my machine)Note
Closes #67