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

Loss value when you click out of the cell #49

Open
nicolagerotto opened this issue Dec 2, 2019 · 12 comments
Open

Loss value when you click out of the cell #49

nicolagerotto opened this issue Dec 2, 2019 · 12 comments
Labels
bug Something isn't working

Comments

@nicolagerotto
Copy link

Hello, I have a strange behavior with the use of the mouse that I found also in the demo.

When a cell is valorized, I enter in editing with the mouse, I do nothing and I click out, the cell loses its value. While if it empties voluntarily it works, but if I don't want it it does the same.

Thanks for your help
Nicola

@nicolagerotto
Copy link
Author

Hi,
any news for this problem?

Regards
Nicola

@avallete
Copy link
Owner

avallete commented Dec 4, 2019

This behaviour have to do with the required option implemented into the isCancelAfterEnd function. I'll investigate it when I have some time.

@nicolagerotto
Copy link
Author

Thanks, can we write in private email address? Because I would urgently need to fix it for the release of a project.

Thanks
Nicola

@avallete
Copy link
Owner

avallete commented Dec 4, 2019

If you need a fix ASAP, you should fork the repo and do the fix you need.

I'll be happy to review the PR you make after that when I have the time to do so.

@zelinsun
Copy link

You could try "onCellValueChanged" event to set it back to the old value.

@nicolagerotto
Copy link
Author

Yes I had already done it, but it is not the desired result. It could be that the cell is actually empty and in that case it's not good. It does not have to assume a value.

@avallete
Copy link
Owner

avallete commented Dec 13, 2019

Finally got some time to think about your issue.

Returning the problem in all the faces, it always get back of 'what is the desired behaviour of required option'. Actually, his behaviour force the cell to always keep a valid value in it, an empty one, isn't a valid value in that logic.

And I don't see how to change this behaviour without breaking other things with side effects.

In an other hand, for your specific case, doing some tests I came up with this solution:

  {
    headerName: "Already present data selector",
    field: "make",
    cellEditor: AutocompleteSelectCellEditor,
    cellEditorParams: {
      selectData: selectData,
      placeholder: "Select an option",
      // Tell the editor that if he previously had a valid value
      // And that if the edition suddenly stop (eg: click outside the editor, esc press, ...)
      // We want to keep the last valid value in it.
      required: true,
      autocomplete: {
        // Override on select, since we want to check the specific case of 'if input is empty string'
        // In that case, we want to create a 'valid empty value' to allow the `required` option to use
        // This new 'empty' value as a valid one
        onSelect: (cellEditor, item, input) => {
          if (input.value === "" && !item) {
            cellEditor.currentItem = {value: null, label: ""};
          } else if (item) {
            cellEditor.currentItem = item;
          }
        }
      }
    },
    valueFormatter: params => {
      // Simply set the formatter to return empty string if both label and value coers to a false result
      if (params.value) {
        return params.value.label || params.value.value || "";
      }
      return "";
    },
    editable: true
  },

That you can test here.

@nicolagerotto Please let me know if it resolve your use case or not.

@ajayupreti
Copy link

I am still facing this issue. I don't want to use the required

@avallete
Copy link
Owner

avallete commented May 5, 2020

@ajayupreti could you please elaborate on why you don't want to use the provided workaround ?

Maybe the workaround discussed here: #57 will suit your use case better ?

You may also try to use the generic ag-Grid ValueSetters and other functions to implement the exact behaviour you want.

PS: I am open to suggestion about how the default behaviour should be, but actually cannot think about another "default" solution since each use-case is pretty specific and I didn't found any proper general formalization of this issue.
To me the best way for the package to deal with it is to keep the choice up to the people using it (that's why I expose cellEditor in callback for instance, it give you full control of the behaviour of the cellEditor)

@avallete avallete reopened this May 5, 2020
@ajayupreti
Copy link

ajayupreti commented May 5, 2020

Hi Andrew,

Thanks for your quick reply.
The example which you shared #49 (comment)
actually I used this piece of code it is clearing value on pressing enter only but not when we clear the value and go out of that cell editor.
same issue in the demo https://stackblitz.com/edit/ag-grid-autocomplete-editor-hvx9ev

How to fix that?

@avallete
Copy link
Owner

avallete commented May 5, 2020

Interesting, this it seem related to the way the click events are handled by ag-Grid.

Basically, only Enter and Tab keys make my component know that the "user validated input", or when clicking on a specific item in the list (which my component specifically handle to call "user validated input" callback).

I'll investigate that when I got some free time.

@avallete avallete added the bug Something isn't working label May 5, 2020
@avallete
Copy link
Owner

After some investigation it's totally related to the way agGrid handle input. A click outside of the cellEditor is not considered like an "input validation", but like an "input canceled" instead.

This might be overridden into autocomplete editor by catching the click event before ag-grid and stopping this propagation. Then triggering the 'input validated' event instead of the 'input canceled' event. Leaving only one way to cancel input (a hit on the Esc key).

Need first to streghten the e2e "natives" tests with cypress-real-events (#93). Then I might introduce this behavior in an option without breaking something else.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants