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

Datagrid: MultiSelect | ShiftClick Feature #2151

Merged
merged 26 commits into from
Apr 15, 2021
Merged

Datagrid: MultiSelect | ShiftClick Feature #2151

merged 26 commits into from
Apr 15, 2021

Conversation

David-Moreira
Copy link
Contributor

Closes #2009

Row appears with text selection on shift click. Should we add : user-select: none; style?
We could either add that style always or only when MultiSelect is active.
image

Note: Removed the @onclick:stopPropagation from the Multi Select Check so we can propagate click arguments and check for Shift Click, this means that everything is now handled on the HandleClick method instead. So there's a new flag _clickFromCheck, to check if the click came from the check and avoid calling into the Single Selection Logic just as we were doing before.

@stsrki
Copy link
Collaborator

stsrki commented Apr 6, 2021

Hi @David-Moreira

I tried the PR and there are some issues. When in the multi-select mode you now need to click on checkbox two times to make it selected. Previously it was working fine (I checked in the live demo).

@David-Moreira
Copy link
Contributor Author

I see. I was testing on Server and that was not happening.
It's happening on WASM. Even though it's multi selecting correctly, the checkbox isn't properly checked.

I'll check it out when I have time. What about the question about the user-select: none; what do you think?

@stsrki
Copy link
Collaborator

stsrki commented Apr 7, 2021

I would leave user-select: none; for now. From user's perspective, they would like to have that option in case they want to copy/paste something.

@David-Moreira
Copy link
Contributor Author

@stsrki fixed.

@stsrki
Copy link
Collaborator

stsrki commented Apr 9, 2021

It still doesn't work. If you click on the same checkbox multiple times without moving a mouse then it will not check or uncheck. If you click, move the mouse, and click again then it works.

@David-Moreira
Copy link
Contributor Author

O.O Alright, I'll take a look again when I have time. Sorry bout that!

@stsrki
Copy link
Collaborator

stsrki commented Apr 9, 2021

No worries :)

@David-Moreira
Copy link
Contributor Author

Holy crap... it looks like that if you keep clicking it stops propagating the click upward into the row.
So it's not being handled by Row Click....
We've had to change to let RowClick handle the checkbox also because of detecting shift click argument....
Any ideias?

@David-Moreira
Copy link
Contributor Author

David-Moreira commented Apr 13, 2021

Ah I figured the culprit... this was driving me crazy. It does not stop propagating, just that the internal click event isn't triggering a Single Click event anymore.
As eventArgs.Detail accumulates the quantity of clicks.

I wasn't aware double click had to be "detected" like this, isn't there a better way to do this?
Could we potentially consider eventargs.Detail > 2 as a single click? Should we actually just track time between clicks which could be more precise as to whether it was a single or double click?
image

@David-Moreira
Copy link
Contributor Author

David-Moreira commented Apr 13, 2021

@stsrki also tested, having the Datagrid set to Single mode and just use Ctrl+Click which selects the row if it isnt selected and de-selects the row if selected, same happens, after 2 clicks on a row, stops triggering click event. Although this is a use case most likely noone would ever try.

I'm guessing this wouldn't be that wrong to do. What do you think?

if ( eventArgs.Detail > 2 ) 
    await Clicked.InvokeAsync( EventArgsMapper.ToMouseEventArgs( eventArgs ) );

Edit: I kinda disregarded the idea of tracking the time between clicks, as delay between the client and the server (on Blazor.Server) can throw if off. We would need to have a check in javascript instead which would then call the server, which I think is an unnecessary bother for this?

@stsrki
Copy link
Collaborator

stsrki commented Apr 14, 2021

Ahhh so that's the reason!

We cannot check for the time between the clicks as that would lead to more complexity than needed. And it would be different than some users have it configured on their own system.

This leads me to think we might go with it and merge as it is. As you said, maybe it is not so wrong...

@David-Moreira
Copy link
Contributor Author

@stsrki Okay, made the change.
Can you take a look?

@stsrki
Copy link
Collaborator

stsrki commented Apr 15, 2021

It still happens but it is good enough. We'll see if there is going to be any complaint and then adjust if needed.

Good job and thank you as always 💪

@stsrki stsrki merged commit 50a3954 into Megabit:dev094 Apr 15, 2021
@stsrki stsrki mentioned this pull request Apr 15, 2021
@@ -287,9 +287,6 @@
<SelectItem TValue="string" Value="@("D")">Diverse</SelectItem>
</Select>
</FilterTemplate>
@code{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

What the hell was this doing here? Did I merge this in? xDD

Copy link
Collaborator

Choose a reason for hiding this comment

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

Might be accidentally merged from my own testings. I tend to throw it randomly like that but this time it slipped unnoticed.

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.

2 participants