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 ability to escape/cancel out of dragging once started #14

Merged

Conversation

oscarssanchez
Copy link
Contributor

@oscarssanchez oscarssanchez commented Mar 14, 2018

Hi @helen

Would this work to solve #12 ?

The functionality works as requested, however, i'm not sure if the code could better fit elsewhere.

@helen
Copy link
Contributor

@helen helen commented Apr 3, 2018

Thanks for the PR, @oscarssanchez! This unfortunately does not seem to work for me - it does cancel the drag in the UI but it still gets updated to the position where you dropped it, which you can then see on a refresh. I see a spinner briefly as well which tells me that the update method is running despite the cancel. Do you see that happening for you?

@oscarssanchez
Copy link
Contributor Author

@oscarssanchez oscarssanchez commented Apr 5, 2018

Hi @helen,

You were right, the update method was still running. I just modified the file so it now stops the updating method if ESC is pressed.

@helen
Copy link
Contributor

@helen helen commented Apr 10, 2018

@oscarssanchez I'm a little bogged down at the moment, so I'm going to kick this over to my coworker @dkotter for more timely review and feedback this week. Thanks for staying on top of this!

@oscarssanchez
Copy link
Contributor Author

@oscarssanchez oscarssanchez commented Apr 11, 2018

Thank you @helen,

Looking forward to keep working on this PR with @dkotter .

@dkotter
Copy link
Contributor

@dkotter dkotter commented Apr 13, 2018

Hey @oscarssanchez, thanks for the work here. Testing this out, it does seem to work for me but I do think there are a few things that can be cleaned up/changed up a bit.

  • For the code that listens for escape key clicks, this should probably live within one of the sortable callback events, so it's not just hanging out by itself at the end. I think the create event probably makes the most sense to use here: http://api.jqueryui.com/sortable/#event-create
  • Within that function, we utilize the keyCode property, which is widely used by all browsers but it's also deprecated, so I think we can modernize this a bit. From what I've found, checking the key property and then falling back to keyCode is a decent way to go, something like:
var key = e.key || e.keyCode;
if ( 'Escape' === key || 'Esc' === key || 27 === key ) {
}

And finally, in my testing, the proper way to stop sorting is by using sortable( 'cancel' ), which you're already doing. But as you probably discovered during your testing, that doesn't stop the update event from firing. Your current code fixes this by setting a class on the table and checking if that exists in that method, which works, but I think we can use sortable's methods here instead.

There's a method to set an option and it's value that I think would work. Within the escape key function, I'd recommend we do something like sortable_post_table.sortable( 'option', 'disabled', true ), along with the sortable( 'cancel' ) you already have.

Then within the update function, check if that value is true (by default it's false): if ( sortable_post_table.sortable( 'option', 'disabled' ) ). If so, set that value back to false (so the sortable still works), and return early there. I think we'd also need to set that value to false in the stop method, in case the update method doesn't fire, which in my testing seemed to happen occasionally.

Let me know if this all makes sense or if you have any questions. Happy to expand further as needed. Thanks!

@oscarssanchez oscarssanchez force-pushed the fix/add-ability-to-cancel-dragging branch from 3dd1c5b to 6635dca Compare Apr 17, 2018
@oscarssanchez oscarssanchez force-pushed the fix/add-ability-to-cancel-dragging branch from 6635dca to 9c7d312 Compare Apr 17, 2018
@helen
Copy link
Contributor

@helen helen commented Apr 17, 2018

@oscarssanchez Hey there - we're still interested in getting this feature into a future release, so if you're unable to keep going with this PR for whatever reason, let me know and we'll take it from here. Thanks!

@oscarssanchez oscarssanchez reopened this Apr 17, 2018
@oscarssanchez
Copy link
Contributor Author

@oscarssanchez oscarssanchez commented Apr 17, 2018

Hi @helen,

I'm sorry about the confusion, I made some mistakes while updating my branch and I ended up erasing my previous commits due to a force push, hence the PR closed automatically. It seems like it's fine now. Thanks! And sorry about that.

@oscarssanchez
Copy link
Contributor Author

@oscarssanchez oscarssanchez commented Apr 17, 2018

Hi @dkotter,

Thank you for your very detailed feedback. Your suggestion is great. I was talking about it with someone and he also suggested solving the problem by adding/removing classes may not have been the best approach.

In my testing with this approach however, setting sortable_post_table.sortable( 'option', 'disabled', true ) works as long as you are performing a dragging operation. But, if the ESC key is pressed while not dragging a page, the sortable disables completely and ends up being unusable, unless you perform a page refresh. My guess is that the update and stop methods won't fire (because we are not performing any dragging), hence sortable_post_table.sortable( 'option', 'disabled' ) can't be set to false again, and since the dragging functions have been disabled altogether, it also can't start the operations again.

From what I read in the documentation, you can add new options and set some values with the optionmethod, so I opted to create a new option called preventUpdate and set it to true on the create method, and to false in the update and stop methods.

That way, the sortable still prevents updating while dragging and does not disable the sorting functionality if the user presses ESC by accident while not dragging a page.

I'm not sure if it may be a good solution, i'll be glad to read more feedback. Thanks!

@dkotter
Copy link
Contributor

@dkotter dkotter commented Apr 18, 2018

@oscarssanchez Just tested this out and it works great, thanks for this! And good catch on the use case of someone using the escape key while not in the process of dragging. Using a custom option there is a much better solution.

I've left two minor CR items that would be nice to add and then if you could, we serve the minified version of the JS file unless the SCRIPT_DEBUG constant is set, so we'll need to compile this src file. Should be able to run npm install and then run grunt to compile that file. If you have issues with that, let me know, I can run that compile step myself.

Thanks again for the help here, looking forward to getting this merged in.

@oscarssanchez
Copy link
Contributor Author

@oscarssanchez oscarssanchez commented Apr 18, 2018

Hi @dkotter, I'm not too familiar with the last step, but no problem I could do it. However, i'm not sure if what you need is the compiled src file or you are going to make some changes before and then compile it. I wanted to check first.

@dkotter
Copy link
Contributor

@dkotter dkotter commented Apr 18, 2018

@oscarssanchez I wasn't planning on making any changes, so when you run the compile step, it will compile the src file you've modified to the minified version. Again, I'm happy to run this step as well, if you'd prefer. If so, I think we're good to go here and I can merge this in.

@@ -88,10 +97,17 @@ sortable_post_table.sortable({
return ui;
},
stop: function(e, ui) {
if ( sortable_post_table.sortable( 'option', 'preventUpdate') ) {
sortable_post_table.sortable( 'option', 'preventUpdate', false );
}
// remove fixed widths
Copy link
Contributor

@dkotter dkotter Apr 18, 2018

Choose a reason for hiding this comment

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

@oscarssanchez Can you add a line break here, just so these sections of code are a little more separated?

if ( sortable_post_table.sortable( 'option', 'preventUpdate') ) {
sortable_post_table.sortable( 'option', 'preventUpdate', false );
return;
}
sortable_post_table.sortable('disable').addClass('spo-updating');
Copy link
Contributor

@dkotter dkotter Apr 18, 2018

Choose a reason for hiding this comment

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

@oscarssanchez Can you add a line break here, just so these sections of code are a little more separated?

@oscarssanchez
Copy link
Contributor Author

@oscarssanchez oscarssanchez commented Apr 19, 2018

Hey @dkotter, I just made the changes.
With regards of the compiling step, it sounds good to me that you take care of it. Let me know if you need anything else. Thanks!

@dkotter
Copy link
Contributor

@dkotter dkotter commented Apr 19, 2018

@oscarssanchez Thanks! Just tested it out and it works great for me. I've compiled that JS file and pushed to your branch. Assigning this over to @helen for final review and merge.

@dkotter dkotter requested a review from helen Apr 19, 2018
@helen
Copy link
Contributor

@helen helen commented May 1, 2018

This looks great! Nice work, all. I'll get this merged and look at the other PR to see if we can do a new release soon.

helen
helen approved these changes May 1, 2018
@helen helen merged commit b37b451 into 10up:develop May 1, 2018
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

3 participants