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

Long-running form submit spinners #1075

Merged
merged 10 commits into from Oct 5, 2015

Conversation

davecranwell
Copy link
Contributor

First pass at addressing #305

910e1569e1eb28b54d385e3077b9dae9

75265d3b273d440d0d1ef22ddcdaaecc

Still to apply this to more forms

@@ -70,7 +70,8 @@ $(function(){
/* close all dropdowns on body clicks */
$(document).on('click', function(e){
var relTarg = e.relatedTarget || e.toElement;
if(!$(relTarg).hasClass('dropdown-toggle')){
if(!$(relTarg).hasClass('dropdown-toggle') && !$(relTarg).closest('.dropdown').length){
console.log($(relTarget).closest('.dropdown'));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will remove

@tomdyson
Copy link
Contributor

Looks good!

@tomdyson
Copy link
Contributor

tomdyson commented Apr 1, 2015

@JoshBarr do you have time to review this?

@davecranwell
Copy link
Contributor Author

  • wagtailembed modal badly needs this spinner. The round trip to embed.ly seems to take forever.

@JoshBarr
Copy link
Contributor

JoshBarr commented Apr 2, 2015

@tomdyson sure thing, I can take a look at the media embedding modal. Are there any others you can think of while I'm there?

@davecranwell davecranwell changed the title WIP: Long-running form submit spinners Long-running form submit spinners Apr 2, 2015
@JoshBarr
Copy link
Contributor

JoshBarr commented Apr 3, 2015

Hey guys,

I was wondering whether we need to use setTimeout to disable the button? I think we can get the parent form from the current node and trigger submit from the click handler. Something like:

// Trigger form submission if it's a submit button
if (this.form && this.getAttribute('type') === 'submit') {
    $(this.form).trigger('submit');
}

// Then carry on and disable the button + bootup the timeout to clear the spinner... 

see it in core.js

Also, I've found an issue with trying to embed multiple media assets into the rich text field (in Chrome at least). If the first element is an embed (no other text nodes at the start of the block), trying to insert a second embed generates an Uncaught HierarchyRequestError: Failed to execute 'insertBefore' on 'Node': Only one element on document allowed. in rangy-core.js:29. I'll take a look at at one next.

@davecranwell
Copy link
Contributor Author

Cheers for reviewing, @JoshBarr. As far as submitting the form goes, I suppose I don't have many issue with that, but I was trying to avoid having to refer to forms at all purely to keep it all within the scope of the button. I was hoping to avoid having to test the button type too, allowing this to be used for ajax forms, synchronous ones, and ones where true "submit" buttons aren't used.

@davecranwell
Copy link
Contributor Author

Ready for merge, I believe.

@gasman
Copy link
Collaborator

gasman commented Jun 24, 2015

There's a problem with the form submit logic from #1075 (comment) - the form submission won't include the name/value of the submit button, as it would when the submit action is triggered directly from the button. We use this to differentiate between the save/publish/submit actions on the page editor - consequently, with this patch applied, the 'publish' button fails to publish. I think this should be reverted to the original setTimeout-based implementation.

@davecranwell
Copy link
Contributor Author

Ah yes, that was the main reason I didn't use form.submit() before, now I remember.

@davecranwell davecranwell self-assigned this Jul 7, 2015
@gasman gasman modified the milestones: 1.2, 1.1 Aug 28, 2015
@davecranwell
Copy link
Contributor Author

Reverted to version that supports passing of button names, also rebases on top of all the gulp & linting stuff which was added after this PR.

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.

None yet

4 participants