-
Notifications
You must be signed in to change notification settings - Fork 69
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
UI: fix spacing on password reset form #495
Conversation
CI seems broken 🥶 can you have a look? |
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.
LGTU! Just the comment about wtforms, we should check all the other invenio deps to ensure that we don't need to pin it there. If you already did just omit the comment.
setup.cfg
Outdated
@@ -40,6 +40,7 @@ install_requires = | |||
pyjwt>=1.5.0 | |||
simplekv>=0.11.2 | |||
ua-parser>=0.7.3 | |||
wtforms<3.2.0 |
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.
Major: Do we know which dependency is bringing wtforms
? we are afraid that it might come from another dependency and we should fix it there instead of pinning it here.
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.
wtforms is at least in invenio-oauth2server and i plan to unpin it there to prepare the code to migrate to flask>=3.0. would it be possible to fix it and not pin it?
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.
I was planning to pin in invenio-admin
instead but if there is a way to fix that would be better :) I personally am not sure how to fix the issue (besides setting the _form_error_key parameter of the form to None)
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.
if you plan to pin it in invenio-admin i am totally fine with it, because there is the plan (and PR's open) to remove the support of invenio-admin from invenio-app-rdm.
where do we would have to fix that problem? in invenio-admin
or in flask-admin
?
@@ -15,7 +15,7 @@ | |||
<div class="ui padded segments big form"> | |||
<div class="ui segment"> | |||
{%- block form_header %} | |||
<h1 class="ui small reset-password header">{{_('Reset password')}}</h1> | |||
<h1 class="ui small header p-25">{{_('Reset password')}}</h1> |
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.
Comment: As you stated in the description we can remove the rule reset-password
rule from invenio-theme. We would recommend doing it now, otherwise we tend to forget 😉
CI should be fixed when inveniosoftware/invenio-admin#106 is merged After this is merged, we should merge this to remove the unnecessary CSS inveniosoftware/invenio-theme#401 |
303f521
to
2e0c63d
Compare
❤️ Thank you for your contribution!
Close #496
Description
Before and after
Before and after
Once this is merged I'll remove the
reset-password
rule as it will no longer be usedChecklist
Ticks in all boxes and 🟢 on all GitHub actions status checks are required to merge:
Frontend
Reminder
By using GitHub, you have already agreed to the GitHub’s Terms of Service including that: