Conversation
fluegelk
left a comment
There was a problem hiding this comment.
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.
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.
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.
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