DataGrid: Cell Navigation support | CellEdit Mode options

David-Moreira commented 1 month ago

Ok so while improving the cell edit feature I realized the following:

Cell navigation can just be a standalone feature. By activating the feature, both single clicking, using arrow keys & tabbing will navigate through the cells. I was initially doing it as part of the cell edit mode, but if it's a standalone feature, the user can just activate whenever he wants. We can recommend to activate it in the docs for cell edit mode.

Introduced a new DataGridEditModeOptions. Here we'll have two new options: CellEditOnSingleClick, CellEditOnDoubleClickthat are self explanatory. Here I would take the breaking change where we default to double click. Users can still enable the old behaviour by setting the CellEditOnSingleClick to true.

Just by adding the extra cell navigation and the single/double click the user has enough flexibility from what I've experienced.

All in all by just adding CellNavigable to a CellEdit enabled Grid the experience is much better!

Let me know your thoughts.

Notes: You will see that we require a new bit of javascript for this feature, I did try to make it work in C#, but as you know, when you have alot of dynamic elements and you have to try to figure out stuff in DOM it gets really hard and cumbersome. I think it's best to do it in js.

David-Moreira commented 1 month ago

Friendly reminder

mtbayley commented 1 month ago

Hi @David-Moreira I've reviewed this branch and I do like the added navigation features.

However, I am wondering if you are still considering adding these two features I mentioned in my feature request. https://github.com/Megabit/Blazorise/issues/5383#issuecomment-2015238362. These features would be extremely valuable for my users.

Typing any value on the keyboard would put the cell in editing mode.

Entering editing mode, selects all text of the current value, allowing a new value to be easily updated

David-Moreira commented 1 month ago

Hello @mtbayley Thanks for reviewing and helping us deliver a better feature.

Both your suggestions seem fine,

Typing any value on the keyboard would put the cell in editing mode.
Entering editing mode, selects all text of the current value, allowing a new value to be easily updated

Would you agree @stsrki ? Do you guys think it's worth keeping these behind an optional flag or it's fine if the grid starts working like this?

Also any of you have any suggestions for this feature? -> "selects all text of the current value" I don't think I personally ever tried something like this, does blazor support this in some way? I'm guessing not? You guys know the js for this?

stsrki commented 1 month ago

Typing any value on the keyboard would put the cell in editing mode.

This seems fine to me. We can do this.

Entering editing mode, selects all text of the current value, allowing a new value to be easily updated

I also don't remember any time doing this. So I would skip it.

The question is how to opt-in. I would add an additional flag to the DataGridEditModeOptions.

mtbayley commented 1 month ago

The question is how to opt-in. I would add an additional flag to the DataGridEditModeOptions.

I agree. Options are good. What might work for me, may not work for someone else.

Also any of you have any suggestions for this feature? -> "selects all text of the current value" I don't think I personally ever tried something like this, does blazor support this in some way? I'm guessing not? You guys know the js for this?

I see Blazorise uses the autoNumeric library to select all on focus for the Numeric Picker component. https://github.com/Megabit/Blazorise/blob/95d0ecda056b515dffc8ebbed07e99ffe17f83a2/Source/Blazorise/wwwroot/numericPicker.js#L116-L118

Can you add something similar to your other input components?

https://github.com/autoNumeric/autoNumeric/blob/ea4a977c230c616fdcda6a5441c6009ce850d2c8/src/AutoNumeric.js#L6421-L6432

_onFocusIn(e) {
    if (this.settings.selectOnFocus) {
        // The whole input content is selected on focus (following the `selectOnFocus` and `selectNumberOnly` options)
        //XXX Firefox <47 does not respect this selection...Oh well.
        this.select();
    } else {
        // Or we decide where to put the caret using the `caretPositionOnFocus` option
        if (!AutoNumericHelper.isNull(this.settings.caretPositionOnFocus)) {
            AutoNumericHelper.setElementSelection(e.target, this._initialCaretPosition(AutoNumericHelper.getElementValue(this.domElement)));
        }
    }
}

https://developer.mozilla.org/en-US/docs/Web/API/HTMLInputElement/select

This is how AG Grid works by default.

msedge_Mb5Ttz4B9L

David-Moreira commented 3 weeks ago

@stsrki Could you evaluate @mtbayley suggestion about the select all focus? Would probably be best to do it on a separate PR and merge it in after.

stsrki commented 3 weeks ago

We have an API on our input that could be used to select all text. But I would leave that for another PR.

David-Moreira commented 3 weeks ago

We have an API on our input that could be used to select all text. But I would leave that for another PR.

Alright, my point is, if we do that feature for 1.6 or not, and wait for it on this PR?

stsrki commented 3 weeks ago

Do it in another PR. For 1.6.

mtbayley commented 3 weeks ago

I appreciate your efforts to get this in 1.6. This will help me out a lot 🙂

David-Moreira commented 3 weeks ago

@stsrki I just realized you said we already support it? Why do we need another PR then?

"We have an API on our input that could be used to select all text."

David-Moreira commented 3 weeks ago

Done. There was indeed already an api for selecting the text. No point in doing anything in a separate PR I guess?

@mtbayley Would you be so kind to take it for a spin if you have time?

mtbayley commented 3 weeks ago

@David-Moreira Looks great! Everything is working how I would expect it to.

Maybe some more clarification in the documentation is what I would add.

Any key to to enter editing mode
Document CellEditSelectTextOnEdit, I think this should be highlighted in the docs since it is such a great feature, and you would only know about it if you stumbled upon it.

David-Moreira commented 3 weeks ago

Great, We'll take a look at your docs suggestions, Thanks.

David-Moreira commented 2 weeks ago

@stsrki Friendly reminder

stsrki commented 2 weeks ago

I'm now testing and it works mostly. But it still need some work. For example, it doesn't work as best with the focus feature.

Focus the cell

2, Hit a key

Hit b key

Notice how only the b is now written. Should it work like that or should it continue writing from a? Maybe we need to add an additional option to control the behavior?

select all on focus
move caret at the end on focus

David-Moreira commented 2 weeks ago

I'm now testing and it works mostly. But it still need some work. For example, it doesn't work as best with the focus feature.

Focus the cell

2, Hit a key

Hit b key

Notice how only the b is now written. Should it work like that or should it continue writing from a? Maybe we need to add an additional option to control the behavior?

select all on focus

move caret at the end on focus

I'm not sure actually. But you can always turn off the Select feature. But maybe the select feature should only select the text if the key pressed was a non text key, like enter or regular click

stsrki commented 2 weeks ago

Actually, from a UX perspective, it makes sense to select all when we double-click to edit the cell. But when we start typing we need to type all characters. Including the first that is now lost.

mtbayley commented 2 weeks ago

I had missed what @stsrki found. I agree, it should not select all on the first character entered. Instead the cursor should be at the end of the character entered (after overwriting previous text).

it makes sense to select all when we double-click to edit the cell

Agree

select the text if the key pressed was a non text key, like enter or regular click

Agree, this how AG grid works.

I also noticed that the Backspace key will clear the cell in AG Grid.

David-Moreira commented 2 weeks ago

Done, thanks for the continuous feedback guys.

It should now only select the text under the same conditions AND if the text hasn't changed.
Backspace now clears
I found some bugs while testing, apparently our UpdateCellEditValue requires the values to be already in the destination type, so we had to try and do some conversions when trying to set strings, ints, decimals, dates, etc..
- Not super happy with this, but when we're already boxing stuff, oh well... It works, that's what it matters. hehe
There was also another bug where apparently the js select function does not exist in a select html element, so it would throw an exception, I added a function check

@stsrki please review again.

stsrki commented 2 weeks ago

I've tested the new feature and it works fantastic. Still need to go review the code.

mtbayley commented 2 weeks ago

Agree. It works great now. Excellent work @David-Moreira

David-Moreira commented 1 week ago

LGTM.

Made some cleaning since I have installed SonarLint installed and it was complaining with some warnings. There are a bunch more I didn't wanted to do them as I don't want to make unwanted bugs. We can do the rest later.

Please check and if you agree I will merge

Yea I'm okay with that, I'd stick to this PR changes as much as possible and do those changes in a separate PR, so we can properly identify them if anything goes wrong.

stsrki commented 1 week ago

LGTM. Made some cleaning since I have installed SonarLint installed and it was complaining with some warnings. There are a bunch more I didn't wanted to do them as I don't want to make unwanted bugs. We can do the rest later. Please check and if you agree I will merge

Yea I'm okay with that, I'd stick to this PR changes as much as possible and do those changes in a separate PR, so we can properly identify them if anything goes wrong.

If you want you can also install SonarLint yourself. It's a free extension for VS.

David-Moreira commented 1 week ago

LGTM. Made some cleaning since I have installed SonarLint installed and it was complaining with some warnings. There are a bunch more I didn't wanted to do them as I don't want to make unwanted bugs. We can do the rest later. Please check and if you agree I will merge

Yea I'm okay with that, I'd stick to this PR changes as much as possible and do those changes in a separate PR, so we can properly identify them if anything goes wrong.

If you want you can also install SonarLint yourself. It's a free extension for VS.

Oh it's free? I might give it a go. :) Thanks for the suggestion.

stsrki commented 1 week ago

Brace yourself for a lot of warnings :) But I guess it is a good thing to be as comply as possible with the standards.

David-Moreira commented 1 week ago

Brace yourself for a lot of warnings :) But I guess it is a good thing to be as comply as possible with the standards.

Depends on who defines those standards, and if they make sense to follow. As always, let's not just blindly follow some tool. haha

Megabit / Blazorise

DataGrid: Cell Navigation support | CellEdit Mode options #5506