Skip to content
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

Add hidden submit button #6472

Merged

Conversation

jimchamp
Copy link
Collaborator

Closes #6471

Adds a hidden submit button prior to the "Delete" submit button.

Technical

A form's first submit button is triggered when enter is pressed. PR #6273 linked the previously broken top-of-page delete button to the form, making it the form's first submit button.

Testing

Screenshot

Stakeholders

@jimchamp jimchamp added the On testing.openlibrary.org This PR has been deployed to testing.openlibrary.org for testing label Apr 22, 2022
@cdrini cdrini added the Patch Deployed This PR has been deployed to production independently, outside of the regular deploy cycle. label Apr 22, 2022
@cdrini cdrini self-assigned this Apr 22, 2022
@@ -34,6 +34,7 @@
note = ""
<h2 class="editFormTitle">$:title</h2>
$if ctx.user and (ctx.user.is_admin() or ctx.user.is_librarian()):
<button type="submit" class="hidden" name="_save" form="addWork" tabindex="-1"></button>
Copy link
Collaborator

@cdrini cdrini Apr 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep; can't find a better solution. Because the consequences of this are a little extreme, let's see if we can avoid this happening in the future!

Could you (1) create a $def like render_submit_button(hidden=False), so that the two submit buttons always have the correct values? I.e. DRY up their rendering logic. No need for this to be a separate file of course!

And (2) try placing the hidden button as the first element in the addWork <form>. That should hopefully prevent this issue from ever happening again!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, and (3) add a comment about why it's necessary; so some future person doesn't "helpfully" delete the "unused hidden button"

@cdrini cdrini added the Priority: 1 Do this week, receiving emails, time sensitive, . [managed] label Apr 22, 2022
@@ -1,4 +1,4 @@
$def with (comment=None)
$def with (comment=None, submit_markup='')
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This macro is referenced in a BUNCH of other places ; we can't modify it like this -- you know unless we do want to delete the Save button on every page but the book page :P It's a good idea to check for usages of a template/function whenever modifying its arguments. I'm going to roll back the last commit 👍

@cdrini cdrini force-pushed the 6471/bug/pressing-enter-should-submit branch from 6402233 to ef5892b Compare April 27, 2022 23:37
@cdrini cdrini force-pushed the 6471/bug/pressing-enter-should-submit branch from 4d46f69 to 29027bc Compare April 27, 2022 23:55
Copy link
Collaborator

@cdrini cdrini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested:

  • ✅ Pressing enter saves for librarians and non-librarians (FF, Chrome, IE)
  • ✅ Pressing delete deletes (FF, Chrome)
    • Doesn't work in IE, but since librarian only feature, doesn't need to be IE-compatible.

@cdrini
Copy link
Collaborator

cdrini commented Apr 27, 2022

@jimchamp The DRY-ing of the submit buttons doesn't appear to be worth it because there's another template in the way, so ignoring. I was able to move the submit button up to the top of the template. I missed that it wasn't inside the <form element, so it needs to be at the top outside the form element.

@cdrini cdrini merged commit d53b9f6 into internetarchive:master Apr 27, 2022
@jimchamp jimchamp deleted the 6471/bug/pressing-enter-should-submit branch July 15, 2022 23:06
@jimchamp jimchamp removed the On testing.openlibrary.org This PR has been deployed to testing.openlibrary.org for testing label Apr 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Patch Deployed This PR has been deployed to production independently, outside of the regular deploy cycle. Priority: 1 Do this week, receiving emails, time sensitive, . [managed]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Book Edit Page: By default, ENTER triggers Delete not Save
2 participants