Form attribute support #802

crhallberg · 2016-09-22T15:21:21Z

Add support for the form="id" attribute on input elements. This new feature is then applied to remove the hack that allowed us to use forms within forms for embedded records.

Unfortunately, IE removed support from conditional loading after IE 9. This means we need to load the polyfill for all browsers and skip the logic addition for those that support it. The asset pipeline makes this negligible for production performance.

demiankatz · 2016-09-26T16:00:53Z

themes/bootstrap3/templates/search/bulk-action-buttons.phtml

@@ -1,25 +1,25 @@
 <? if((isset($this->showBulkOptions)  && $this->showBulkOptions)
   || (isset($this->showCartControls) && $this->showCartControls)): ?>
-  <div class="bulkActionButtons hidden-print">
+  <div class="bulkActionButtons hidden-print<?if($this->idPrefix):?> form-inline<? endif; ?>">


Do you think it might make more sense to include a separate attribute for form name, rather than checking idPrefix? We can rely on idPrefix based on current usage, but it's not really an intuitively logical trigger for form attribute functionality.

Looks like you've changed most of the new idPrefix checks to formAttr checks, except for the one right above these comments. Is that intentional or a typo?

demiankatz · 2016-09-26T16:10:44Z

I can't seem to add items to my cart in Firefox 49 when using this branch.

crhallberg · 2016-09-27T18:07:38Z

This branch needs the love of the cart fixes.

Conflicts: themes/bootstrap3/js/cart.js

crhallberg

Thanks!

crhallberg · 2016-09-28T19:40:29Z

We have a new version of solrmarc being added here. Is that a GitHub weirdness or is that from a merge from master?

demiankatz · 2016-09-29T01:36:23Z

That's valid -- it was recently updated to fix a bug.

Conflicts: themes/bootstrap3/js/cart.js themes/bootstrap3/js/hold.js

demiankatz · 2016-09-30T18:22:06Z

@crhallberg, this branch seems to have developed a conflict with master. Could you sort that out when you have a chance? Once it's up to date, I'll give it another round of testing. Thanks!

Conflicts: themes/bootstrap3/templates/search/results.phtml

demiankatz · 2016-10-06T18:34:28Z

themes/bootstrap3/js/lightbox.js

@@ -278,7 +278,10 @@ VuFind.register('lightbox', function Lightbox() {
      data: data
    }).done(function recaptchaReset() {
      if (typeof grecaptcha !== 'undefined') {
-        grecaptcha.reset($(form).find('.g-recaptcha').data('captchaId'));
+        var captcha = $(form).find('.g-recaptcha');


This captcha-related change appears to pop up several times in this PR. Is this related to the form change, or is this an unrelated improvement? Do we perhaps want a resetReCaptcha(form) function in common.js so we don't have to repeat the same four lines of code multiple times, or is that overkill?

demiankatz · 2016-10-06T18:37:59Z

themes/bootstrap3/theme.config.php

@@ -15,6 +15,7 @@
        'vendor/bootstrap.min.js',
        'vendor/bootstrap-accessibility.min.js',
        'vendor/validator.min.js',
+        'vendor/form-attr-polyfill.js', // input[form] polyfill, cannot load conditionally, since we need all versions of IE


Does this really belong in the vendor directory if you wrote it? At very least, should it get its own repo somewhere in addition to being dropped here?

I did not write it, I found it online and updated it. I think maybe we need a new folder for the elements that stand alone, but are custom to VuFind. They exist between vendor and regular scripting. Like autocomplete and the new channels slider.

Might "lib" be an appropriate directory name for this sort of thing?

crhallberg · 2016-10-14T19:24:43Z

Note: Changing the line your comment was on removed your comment.

Changes requested have been addressed.

Conflicts: themes/bootstrap3/js/record.js themes/bootstrap3/theme.config.php

demiankatz · 2016-11-08T13:48:28Z

@crhallberg, where are we on this one? A couple of questions that stand out from a quick review:

1.) I notice that you adjusted _registerUpdate() to have a hard-coded form retrieval inside it, rather than accepting a form as a parameter. Does this reduce any important flexibility?

2.) There's some discussion above about the location of form-attr-polyfill.js, since right now it's not technically a vendor file, since it only exists in its current form as part of VuFind. Do we need to continue that conversation?

crhallberg · 2016-11-08T16:26:13Z

1.) I don't expect this will be any less flexible, but I can add a form option back in for devs who want to bind cart submission to another form.
2.) I think form-attr-polyfill.js may be a good candidate for our new lib folder. I did find the code online, but it was heavily updated.

crhallberg · 2016-12-13T21:05:04Z

Found a few IE bugs. Working on them. See checkboxes in description.

- Remove previously added fields for multiple form submissions. - Removed extraneous jQuery plugin appendFields. - Faster in IE.

crhallberg · 2016-12-13T22:07:09Z

Can anyone confirm or explain why IE would be taking so long to get to the Print interface?

Turn on bulk actions
Do a search
Check a few boxes
Print from bulk toolbar

It's slow on both master and this branch, taking up to 10 seconds on my machine.

demiankatz · 2016-12-14T13:29:12Z

Have you tried replacing the actual print operation with some arbitrary task, like an alert? I'd be interested to know whether the slowness was specifically related to IE's print functionality (in which case, there's not much we can do about it), or if the problem is happening somewhere leading up to that point.

crhallberg · 2016-12-14T16:07:54Z

It's using the same form and method as email and export, this leads me to think it is the print process specifically. Also, this is happening in master as well.

crhallberg · 2016-12-14T16:51:47Z

I did just replace the print with an alert and it was immediate, no pause at all. The delay today isn't as bad, which is also why I was looking for confirmation.

Chris Hallberg and others added 13 commits September 21, 2016 17:56

Add polyfill.

049c0f8

Add form attributes to bottom buttons and checkboxes in search.

791a0a8

Adjust checkbox-select-all to allow for [form] attributes.

67d5f0c

Adjust unit test.

fc0e2bc

Merge remote-tracking branch 'origin/master' into form-attr-cart

67fe24f

Remove magic script form retention hack.

246b2c4

Expand _clickedButton logic in lightbox to include [form] attrs.

233e16f

Eslint. Remove console.log.

ecdf8f0

Merge remote-tracking branch 'origin/master' into form-attr-cart

f6e44e2

grecaptcha reset fix.

9b5572e

Polyfill fixed to run on document ready.

ceeb2a6

Merge branch 'master' into form-attr-cart

2d5763d

removed <script> hack

b228715

demiankatz previously requested changes Sep 26, 2016

View reviewed changes

Find form attributed checkboxes as well for search cart adding.

655f8c9

Chris Hallberg added 2 commits September 27, 2016 14:23

Merge remote-tracking branch 'origin/release-3.1' into form-attr-cart

f1c6954

Conflicts: themes/bootstrap3/js/cart.js

eslint

26db220

crhallberg commented Sep 27, 2016

View reviewed changes

Chris Hallberg added 2 commits September 29, 2016 12:54

Merge branch 'master' into form-attr-cart

f158fee

Conflicts: themes/bootstrap3/js/cart.js themes/bootstrap3/js/hold.js

formAttr parameter for bulk-action-buttons.

3fb1d45

Merge remote-tracking branch 'origin/master' into form-attr-cart

328cc82

Conflicts: themes/bootstrap3/templates/search/results.phtml

demiankatz reviewed Oct 6, 2016

View reviewed changes

Switch idPrefix to formAttr in one missed place.

4fb2cae

crhallberg added the new feature label Oct 14, 2016

Chris Hallberg added 2 commits October 21, 2016 17:17

Consolidate recaptcha resetting logic.

0088e6c

Merge remote-tracking branch 'origin/master' into form-attr-cart

6ac9da3

Conflicts: themes/bootstrap3/js/record.js themes/bootstrap3/theme.config.php

Chris Hallberg added 2 commits November 8, 2016 12:01

Returning some flexibility to cart update button submission.

fea6d6b

Move form-attr-polyfill to /lib

9d8d7bf

crhallberg self-assigned this Nov 22, 2016

Merge remote-tracking branch 'origin/master' into form-attr-cart

25fdfdf

Chris Hallberg added 3 commits December 13, 2016 16:36

Improve form-attr-polyfill.js

649347a

- Remove previously added fields for multiple form submissions. - Removed extraneous jQuery plugin appendFields. - Faster in IE.

Remove unnecessary class.

e97311f

Pass in formAttr for bottom search bulk action form.

bc5be43

Whitespace fix.

0fe706d

demiankatz merged commit f61ded2 into vufind-org:master Dec 15, 2016

crhallberg mentioned this pull request Jun 5, 2017

Decolumnize layouts (BS3 reduce) #978

Merged

7 tasks

crhallberg deleted the form-attr-cart branch June 26, 2017 17:04

maccabeelevine mentioned this pull request Nov 14, 2023

Combined: Fix scope of search cart form #3219

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Form attribute support #802

Form attribute support #802

crhallberg commented Sep 22, 2016 •

edited

Loading

demiankatz Sep 26, 2016

demiankatz Oct 6, 2016

demiankatz commented Sep 26, 2016

crhallberg commented Sep 27, 2016

crhallberg left a comment

crhallberg commented Sep 28, 2016

demiankatz commented Sep 29, 2016

demiankatz commented Sep 30, 2016

demiankatz Oct 6, 2016

demiankatz Oct 6, 2016

crhallberg Oct 14, 2016 •

edited

Loading

demiankatz Oct 18, 2016

crhallberg commented Oct 14, 2016

demiankatz commented Nov 8, 2016

crhallberg commented Nov 8, 2016

crhallberg commented Dec 13, 2016

crhallberg commented Dec 13, 2016

demiankatz commented Dec 14, 2016

crhallberg commented Dec 14, 2016 •

edited

Loading

crhallberg commented Dec 14, 2016

Form attribute support #802

Form attribute support #802

Conversation

crhallberg commented Sep 22, 2016 • edited Loading

demiankatz Sep 26, 2016

Choose a reason for hiding this comment

demiankatz Oct 6, 2016

Choose a reason for hiding this comment

demiankatz commented Sep 26, 2016

crhallberg commented Sep 27, 2016

crhallberg left a comment

Choose a reason for hiding this comment

crhallberg commented Sep 28, 2016

demiankatz commented Sep 29, 2016

demiankatz commented Sep 30, 2016

demiankatz Oct 6, 2016

Choose a reason for hiding this comment

demiankatz Oct 6, 2016

Choose a reason for hiding this comment

crhallberg Oct 14, 2016 • edited Loading

Choose a reason for hiding this comment

demiankatz Oct 18, 2016

Choose a reason for hiding this comment

crhallberg commented Oct 14, 2016

demiankatz commented Nov 8, 2016

crhallberg commented Nov 8, 2016

crhallberg commented Dec 13, 2016

crhallberg commented Dec 13, 2016

demiankatz commented Dec 14, 2016

crhallberg commented Dec 14, 2016 • edited Loading

crhallberg commented Dec 14, 2016

crhallberg commented Sep 22, 2016 •

edited

Loading

crhallberg Oct 14, 2016 •

edited

Loading

crhallberg commented Dec 14, 2016 •

edited

Loading