Skip to content

Switch to forms#22

Open
bt90 wants to merge 3 commits intonosduco:mainfrom
bt90:login_form
Open

Switch to forms#22
bt90 wants to merge 3 commits intonosduco:mainfrom
bt90:login_form

Conversation

@bt90
Copy link
Contributor

@bt90 bt90 commented Aug 15, 2023

This PR switches the page to use a form to group the inputs. This allows the required field and enter key handling to work without Javascript.

@nosduco
Copy link
Owner

nosduco commented Aug 15, 2023

Thanks for your countless contributions! You're a real pioneer of this project :)

I have some questions about this PR though, what are the benefits of switching to a form? I understand that not everyone wants to run Javascript, mainly for security reasons.

@bt90
Copy link
Contributor Author

bt90 commented Aug 16, 2023

The change is mostly semantic as it groups the input elements in a form to tell the browser that they belong together. The benefit is that things work the way the user expects them to, without having to manually handle things we used to do with custom js.

  • the required attribute of the username/password fields is enforced by the browser itself. The user can't submit empty fields and instead gets a nicely localized warn message for each field
  • since it's a single form, it can be submitted via Enter key and we don't need to check for the key event anymore

While not strictly part of the form rework, I also changed the js code to reenable the inputs in the event of an unknown error.

Feel free to test it. As it's a bit bigger change and I'm not very familiar with javascript 😅

@bt90
Copy link
Contributor Author

bt90 commented Aug 16, 2023

We could also go one step further and allow the user to submit the form without having javascript enabled. But that would require changes to /login endpoint in the backend as it would need to handle application/x-www-form-urlencoded form data.

@bt90
Copy link
Contributor Author

bt90 commented Aug 16, 2023

The best way would probably to handle the POST of the form in / and get rid of /login:

<form action="/" method="post">

@nosduco
Copy link
Owner

nosduco commented Aug 18, 2023

Understood! Yeah, I was thinking of potentially trying to remove all JS from it... will test out your branch and explore what may be possible. Thanks again for your countless contributions :)

@bt90
Copy link
Contributor Author

bt90 commented Aug 18, 2023

We can merge the current state and try to replace the JS based logic later on.

@nosduco nosduco changed the base branch from dev to main November 2, 2023 04:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants