vitalets / angular-xeditable

Edit in place for AngularJS
http://vitalets.github.io/angular-xeditable
MIT License
1.91k stars 402 forks source link

Control deselects when navigating outside the area - mouseup on outside #757 #758

Closed drsmog closed 5 years ago

drsmog commented 5 years ago

issue - #757

problem: when you are doing mouse up outside on input box, submit is triggering. reason: because we are listening on click event - line: 1485 - $document.bind('click', function (e) { .... solution: listen to mousedown like - $document.bind('mousedown', function (e) {

ckosloski commented 5 years ago

Did all the tests pass?

drsmog commented 5 years ago

Did all the tests pass?

unfortunately, nope some tests are failing even without changes (just pull it from master to check original one and still getting some failure tests - example in dev tests - describe: uidate 3046ms should show editor and submit new value)

ckosloski commented 5 years ago

The master tests and the tests with your change work fine for me. But, when I test functionality, it is off. You are correct, the select and hold no longer triggers the cancel and closes the element. If I select and then click outside the element to cancel editing, it doesn't close the element. I have to click twice outside the element in order to trigger the cancel event.

ckosloski commented 5 years ago

What if you changed your blur event to "ignore", maybe that would work better for you? http://vitalets.github.io/angular-xeditable/#ref-element

drsmog commented 5 years ago

What if you changed your blur event to "ignore", maybe that would work better for you? http://vitalets.github.io/angular-xeditable/#ref-element @ckosloski ah I guess nope in that case as I mentioned it will create another usability issue. so, in that case, clicking on some apply button would be mandatory and it would be a little bit confusing for our user so we prefer to avoid that scenario.

in short, we are ok with behavior that mousedown does. the only blocker for now as I understand is that tests are failing (because all test scenarios is using click event instead of mousdown). is it ok to fix those tests and merge the PR after that?

ckosloski commented 5 years ago

As I said, the tests were passing for me. But having to click twice outside of the element in order to cancel the element may not be something other people want. Maybe you can add a setting that would set the event binding to click or mousedown depending on the setting?

drsmog commented 5 years ago

@ckosloski yeah thats nice ill add a settings

image

any ideas how to access $attrs there im getting an error https://code.angularjs.org/1.5.0/docs/error/$injector/unpr?p0=$attrsProvider%20%3C-%20$attrs%20%3C-%20editableFormController%20%3C-%20editableDirectiveFactory%20%3C-%20editableTextDirective

could u please provide some hints ?

ckosloski commented 5 years ago

You could add the setting to editableOptions if you want it to be global. The code you pasted is for the form controller and not the element. I think you would have to do something similar to line 194 in order to get a specific attributes: this.$editables[0].elem[0].selectionStart

drsmog commented 5 years ago

@ckosloski just have updated PR could u please review it again i've add new attribute - usemousedown with values "true|false" there is a video record how it works - https://www.loom.com/share/2d3f5da4b82546caac060c540b793248

ckosloski commented 5 years ago

You need to update the documentation to add this new attribute. Similar to the "blur" documentation at lines 82-90 in editable-element/controller.js

drsmog commented 5 years ago

@ckosloski done

drsmog commented 5 years ago

@ckosloski can we merge that?

ckosloski commented 5 years ago

The tests all pass for me and the doc site seems to work fine. @eugef take a look and merge if it looks good to you.

eugef commented 5 years ago

Hi @ckosloski, I am currently on vacation and can't access my laptop . Will merge and release in one week or so.

drsmog commented 5 years ago

Hi @eugef. any chance to merge it today?

eliOcs commented 5 years ago

Anything we can do from our side to help to get this merged?

eugef commented 5 years ago

Sorry for the delay, new version 0.10.1 is released

bartdk-be commented 5 years ago

@eugef : Might be a good idea to scan console.logs before creating a release ? Or we could use a logger and strip out console logs when going to production

See : https://github.com/vitalets/angular-xeditable/pull/758/commits/651d6e4fa738f26d3a98152faa78d145ef10285b#diff-f5f66c70e9eaebe7cf34be973bd5b977R51

ckosloski commented 5 years ago

@drsmog can you create a new PR removing the console.log that was added?

bartdk-be commented 5 years ago

@drsmog :

Will wait for future update. Thanks in advance !

eugef commented 5 years ago

I would go for eslint rule that checks against console.log

eugef commented 5 years ago

New version 0.10.2 that fixes this issue is released

bartdk-be commented 5 years ago

@eugef / @drsmog : Thanks We are still unable to use this feature, as it's not exposed to the editableOptions and require us to change all our control usages. Are you still planning to introduce this option ? Just so we know whether we should implement a workaround.

eugef commented 5 years ago

@bartdk, your are welcome to create a PR that exposes this feature