Disable cover upload `submit` button while uploading #8754

rebecca-shoptaw · 2024-01-24T21:24:58Z

Feature. Adds a js function to disable Submit button and replace text with Uploading... when form is submitted.

Technical

The addLoadingStyling function is currently defined locally within the on submit function for #addcover-form. Could be moved into the initCoversAddManage function or elsewhere in the case of re-use (i.e. for the Save button in the manage cover window).

Another note: there is no function to clear the styling because the template is re-rendered when the function returns; if re-used elsewhere, a simple cleanup function may be necessary.

With @cdrini's help, I settled on the data-loading-text attribute to store the "Uploading..." message for translation purposes. Any other implementation of the function would require the same attribute in the corresponding HTML.

Testing

Log in (if not logged in)
Go to a book page
Hover over the book and select Manage Covers
Hit the Submit button in the Add tab

Screenshot

Stakeholders

@cdrini

rebecca-shoptaw · 2024-01-24T21:33:32Z

Alas it seems I ran into some problems with the linter because I called my new function above the var declarations of the form values. I opted to do this because I noticed that an invalid image URL would cause the function to exit early at this declaration and thus not run the styling function.

Any advice re: the best way to resolve this would be wonderful!

Note: it seems to be the .trim() within the val function that causes the error (when the selector value is undefined):

function val(selector) {
    return $(selector).val().trim();
}

Preventing this error from happening (i.e. by ensuring the $(selector).val() exists before trimming it should hypothetically allow me to switch the order back.

cdrini

Nice! This looks great! One change -- should also fix the linting issue.

cdrini · 2024-01-24T21:44:31Z

openlibrary/plugins/openlibrary/js/covers.js

+            const btn = $('#imageUpload');
+            btn.prop('disabled', true).html(btn.data('loading-text'));


This doesn't need to be in a function; you can move these lines down to just after 83. That will also let the validation checks pass first before disabling the button.

We want those error checks to run first; because if it returns it won't actually upload the image, so we don't want to show the loading text.

So I think you will have to fix the val bug!

Sounds good to me! I'll go ahead and make the changes here, then have a look at the val issue tomorrow.

Moved everything and it all seems to be working great; just wanted to double check whether the val fix should be done here as well or as a separate issue/PR. Thanks so much!

Oh here is good! Should be a quick fix :) Note you'll have to do a rebase ontop of the lastest master since the other covers PR touches similar code 👍

Great! Will finish everything up in the morning!

Done! And fixing the val bug made it clear that the checks to return an error if no file/url/id had been submitted - if (file === '' && url == '' && coverid == '') - had been failing the whole time because the coverid val defaults to undefined and not ''. So I threw in a quick fix for that as well by switching to if (!file && !url && !coverid), so now it looks a bit cleaner and the correct error message shows if nothing is entered.

Nice! Much cleaner 🥳

codecov-commenter · 2024-01-25T00:01:10Z

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (6617494) 16.62% compared to head (5caec20) 16.61%.

Files	Patch %	Lines
openlibrary/plugins/openlibrary/js/covers.js	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8754      +/-   ##
==========================================
- Coverage   16.62%   16.61%   -0.01%     
==========================================
  Files          88       88              
  Lines        4698     4700       +2     
  Branches      837      837              
==========================================
  Hits          781      781              
- Misses       3399     3401       +2     
  Partials      518      518

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…xt on form submit.

cdrini

Lgtm! Thanks @rebecca-shoptaw ! Tested uploading, upload causing an error, upload via upload vs via url.

jimchamp · 2024-02-27T20:31:30Z

openlibrary/templates/covers/add.html

@@ -68,7 +68,9 @@
            </div>

            <div class="formElement" style="margin: $('15px 0px 0px 15px;' if doc.type.key == '/type/work' else '0px;')">
-                <button type="submit" name="upload" id="imageUpload" value="$_('Submit')" class="largest">$_("Submit")</button>
+                <button type="submit" name="upload" id="imageUpload" value="$_('Submit')" class="largest" data-loading-text='$_("Uploading...")'>


Let's stick to using data-i18n for localized strings.

Let's chat about it during our ABC call! We use a few different patterns in the code, +1 it would be good to standardize on one.

How do I find the other patterns?

I think I've seen 3 patterns?

data-i18n

one-offs (like here)

IIRC some parts of the codebase have like i18n in a separate <script> tag? I can't recall

It's a touch hard to search for! I think data-i18n is a reasonable standard should we choose to adopt it :)

Here are some examples I found of 2/3

2:

openlibrary/openlibrary/templates/type/list/view_body.html

Line 60 in 7fe6b53

<a href="javascript:;" data-confirm-text="$:_('Yes, I\'m sure')" data-cancel-text="$_('No, cancel')"><span></span>$_('Remove this item?')</a>

3:

openlibrary/openlibrary/templates/lists/widget.html

Line 69 in 4ba4f96

<input type="hidden" name="list-i18n-strings" value="$json_encode(show_list_i18n_strings)">

oof... if I ever find the guy that implemented #3...

Converting these to use data-i18n could be a good first issue (or issues).

cdrini self-assigned this Jan 24, 2024

cdrini requested changes Jan 24, 2024

View reviewed changes

rebecca-shoptaw force-pushed the 8749/disable-cover-submit-btn-onclick branch from 23440c1 to 8ea5a81 Compare January 24, 2024 23:58

rebecca-shoptaw added 4 commits January 26, 2024 12:56

Add loading message data attribute to cover add HTML.

0c68216

Create and call function to disable Submit button and show loading te…

c58256b

…xt on form submit.

Move uploading button styling so it runs after error checks.

7234956

Prevent val function from causing type error.

3e9e522

rebecca-shoptaw force-pushed the 8749/disable-cover-submit-btn-onclick branch from 5caec20 to 3e9e522 Compare January 26, 2024 18:42

cdrini added the On testing.openlibrary.org This PR has been deployed to testing.openlibrary.org for testing label Jan 27, 2024

cdrini approved these changes Jan 27, 2024

View reviewed changes

cdrini merged commit d05f2dc into internetarchive:master Jan 27, 2024
4 checks passed

rebecca-shoptaw mentioned this pull request Feb 27, 2024

Fix untranslated error messages in add.html and web.html. #8848

Closed

jimchamp reviewed Feb 27, 2024

View reviewed changes

jimchamp removed the On testing.openlibrary.org This PR has been deployed to testing.openlibrary.org for testing label Apr 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disable cover upload `submit` button while uploading #8754

Disable cover upload `submit` button while uploading #8754

rebecca-shoptaw commented Jan 24, 2024

rebecca-shoptaw commented Jan 24, 2024 •

edited

cdrini left a comment

cdrini Jan 24, 2024 •

edited

rebecca-shoptaw Jan 24, 2024

rebecca-shoptaw Jan 25, 2024 •

edited

cdrini Jan 25, 2024

rebecca-shoptaw Jan 25, 2024 •

edited

rebecca-shoptaw Jan 26, 2024 •

edited

cdrini Jan 27, 2024

codecov-commenter commented Jan 25, 2024

cdrini left a comment •

edited

jimchamp Feb 27, 2024

cdrini Feb 27, 2024

jimchamp Feb 27, 2024

cdrini Feb 27, 2024

cdrini Feb 27, 2024

jimchamp Feb 27, 2024

		const btn = $('#imageUpload');
		btn.prop('disabled', true).html(btn.data('loading-text'));

Disable cover upload submit button while uploading #8754

Disable cover upload submit button while uploading #8754

Conversation

rebecca-shoptaw commented Jan 24, 2024

Technical

Testing

Screenshot

Stakeholders

rebecca-shoptaw commented Jan 24, 2024 • edited

cdrini left a comment

Choose a reason for hiding this comment

cdrini Jan 24, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rebecca-shoptaw Jan 25, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rebecca-shoptaw Jan 25, 2024 • edited

Choose a reason for hiding this comment

rebecca-shoptaw Jan 26, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Jan 25, 2024

Codecov Report

cdrini left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Disable cover upload `submit` button while uploading #8754

Disable cover upload `submit` button while uploading #8754

rebecca-shoptaw commented Jan 24, 2024 •

edited

cdrini Jan 24, 2024 •

edited

rebecca-shoptaw Jan 25, 2024 •

edited

rebecca-shoptaw Jan 25, 2024 •

edited

rebecca-shoptaw Jan 26, 2024 •

edited

cdrini left a comment •

edited