-
-
Notifications
You must be signed in to change notification settings - Fork 531
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
DataGrid: Cell Navigation support | CellEdit Mode options #5506
DataGrid: Cell Navigation support | CellEdit Mode options #5506
Conversation
Friendly reminder |
Documentation/Blazorise.Docs/Pages/Docs/Extensions/DataGrid/DataGridApi.razor
Outdated
Show resolved
Hide resolved
Hi @David-Moreira However, I am wondering if you are still considering adding these two features I mentioned in my feature request. #5383 (comment). These features would be extremely valuable for my users.
|
Hello @mtbayley Both your suggestions seem fine,
Would you agree @stsrki ? Also any of you have any suggestions for this feature? |
This seems fine to me. We can do this.
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 |
I agree. Options are good. What might work for me, may not work for someone else.
I see Blazorise uses the autoNumeric library to select all on focus for the Numeric Picker component. Blazorise/Source/Blazorise/wwwroot/numericPicker.js Lines 116 to 118 in 95d0ecd
Can you add something similar to your other input components?
https://developer.mozilla.org/en-US/docs/Web/API/HTMLInputElement/select This is how AG Grid works by default. |
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? |
Do it in another PR. For 1.6. |
I appreciate your efforts to get this in 1.6. This will help me out a lot 🙂 |
@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." |
Done. @mtbayley Would you be so kind to take it for a spin if you have time? |
@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.
|
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 |
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. |
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).
Agree
Agree, this how AG grid works. I also noticed that the |
…and select are actual functions before invoking | Handle text vs numeric according to column type when updating cell edit
Done, thanks for the continuous feedback guys.
@stsrki please review again. |
I've tested the new feature and it works fantastic. Still need to go review the code. |
Agree. It works great now. Excellent work @David-Moreira |
…rd-when-in-editmode=datagrideditmodecell
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
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 |
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
,CellEditOnDoubleClick
that 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.