fix(lib): start selection inbetween select items #167
Conversation
|
Hi @d3lm thank you for this great library! :) As this is my first PR in Github I'm sure there's going to be a couple of things I should fix. So please just let me know if you've got any feedback 👍 |
|
Thanks! And also thanks for your PR, really appreciate it. I ll have a look at your PR 👍 |
|
Hi @d3lm, why this PR not yet accepted. I checked this and it's working as expected. I need this change for my project. So give this feature ASAP. |
|
Hey there! If you really need this you always have the option to fork it. I am not maintaining this library in a full-time nor part-time capacity at the moment and I have many other responsibilities so I ask for a bit of understanding. I just didn't have the time to look at this PR yet. I am afraid that being pushy won't help much here. |
cypress/integration/dragging.spec.ts
Outdated
| @@ -91,6 +93,82 @@ describe('Dragging', () => { | |||
| .dispatch('mouseup'); | |||
| }); | |||
| }); | |||
|
|
|||
| describe('selection with dragOverItems set to false', () => { | |||
There was a problem hiding this comment.
| describe('selection with dragOverItems set to false', () => { | |
| describe('Selection with dragOverItems set to false', () => { |
cypress/integration/dragging.spec.ts
Outdated
| }); | ||
| }); | ||
|
|
||
| it('should start selection in element inbetween SelectContainer and SelectItem', () => { |
There was a problem hiding this comment.
| it('should start selection in element inbetween SelectContainer and SelectItem', () => { | |
| it('should start selection on element inbetween SelectContainer and SelectItem', () => { |
cypress/integration/dragging.spec.ts
Outdated
| }); | ||
| }); | ||
|
|
||
| describe('selection with selectOnClick set to false', () => { |
There was a problem hiding this comment.
| describe('selection with selectOnClick set to false', () => { | |
| describe('Selection with selectOnClick set to false', () => { |
cypress/integration/dragging.spec.ts
Outdated
| }); | ||
| }); | ||
|
|
||
| it('should start selection in element inbetween SelectContainer and SelectItem', () => { |
There was a problem hiding this comment.
| it('should start selection in element inbetween SelectContainer and SelectItem', () => { | |
| it('should start selection on element inbetween SelectContainer and SelectItem', () => { |
cypress/integration/dragging.spec.ts
Outdated
| cy.get('mat-grid-list') | ||
| .as('end') | ||
| .scrollIntoView() | ||
| .wait(16) |
There was a problem hiding this comment.
Why are all these manual wait points necessary? Generally I'd say it's not ideal especially because Cypress has automatic wait points built-in in almost all APIs. Can you elaborate why these are needed?
There was a problem hiding this comment.
Just removed the waits and it's working just fine... not sure if these were required back when I created the PR but I probably just copypasta'd from
without really looking/understanding the above comment. So I guess it's not necessary for my test scenario?Plus I'm not familiar with Cypress, only been using Playwright so far 🙈
| @@ -680,4 +683,13 @@ export class SelectContainerComponent implements AfterViewInit, OnDestroy, After | |||
|
|
|||
| return null; | |||
| } | |||
|
|
|||
| private _isClickOutsideSelectableItem(element: EventTarget): boolean { | |||
| if (!(element instanceof HTMLElement)) return false; | |||
There was a problem hiding this comment.
| if (!(element instanceof HTMLElement)) return false; | |
| if (!(element instanceof HTMLElement)) { | |
| return false; | |
| } |
| private _isClickOutsideSelectableItem(element: EventTarget): boolean { | ||
| if (!(element instanceof HTMLElement)) return false; | ||
|
|
||
| if (element === this.host) return true; |
| if (!(element instanceof HTMLElement)) return false; | ||
|
|
||
| if (element === this.host) return true; | ||
| if (this._selectableItemsNative.includes(element)) return false; |
|
Hey there! Thanks for having a look at my PR. I'll try to have a look in
the evening =)
Dominic Elm ***@***.***> schrieb am So., 28. Juli 2024, 10:34:
… ***@***.**** requested changes on this pull request.
Thanks for the PR. Left some minor remarks.
------------------------------
In cypress/integration/dragging.spec.ts
<#167 (comment)>
:
> @@ -91,6 +93,82 @@ describe('Dragging', () => {
.dispatch('mouseup');
});
});
+
+ describe('selection with dragOverItems set to false', () => {
⬇️ Suggested change
- describe('selection with dragOverItems set to false', () => {
+ describe('Selection with dragOverItems set to false', () => {
------------------------------
In cypress/integration/dragging.spec.ts
<#167 (comment)>
:
> + disableDragOverItems().then(() => {
+ getDesktopExample().within(() => {
+ cy.getSelectItem(0)
+ .dispatch('mousedown', { button: 0 })
+ .getSelectItem(6, 'end')
+ .dispatch('mousemove')
+ .shouldSelect([1])
+ .getSelectBox()
+ .then(shouldBeInvisible)
+ .***@***.***')
+ .dispatch('mouseup');
+ });
+ });
+ });
+
+ it('should start selection in element inbetween SelectContainer and SelectItem', () => {
⬇️ Suggested change
- it('should start selection in element inbetween SelectContainer and SelectItem', () => {
+ it('should start selection on element inbetween SelectContainer and SelectItem', () => {
------------------------------
In cypress/integration/dragging.spec.ts
<#167 (comment)>
:
> + .wait(16)
+ .trigger('mousedown', 210, 50, { button: 0 })
+ .wait(16)
+ .getSelectItem(6)
+ .dispatch('mousemove')
+ .shouldSelect([2, 3, 6, 7])
+ .getSelectBox()
+ .then(shouldBeVisible)
+ .***@***.***')
+ .dispatch('mouseup');
+ });
+ });
+ });
+ });
+
+ describe('selection with selectOnClick set to false', () => {
⬇️ Suggested change
- describe('selection with selectOnClick set to false', () => {
+ describe('Selection with selectOnClick set to false', () => {
------------------------------
In cypress/integration/dragging.spec.ts
<#167 (comment)>
:
> + disableSelectOnClick().then(() => {
+ getDesktopExample().within(() => {
+ cy.getSelectItem(0)
+ .dispatch('mousedown', { button: 0 })
+ .getSelectItem(6, 'end')
+ .dispatch('mousemove')
+ .shouldSelect([])
+ .getSelectBox()
+ .then(shouldBeInvisible)
+ .***@***.***')
+ .dispatch('mouseup');
+ });
+ });
+ });
+
+ it('should start selection in element inbetween SelectContainer and SelectItem', () => {
⬇️ Suggested change
- it('should start selection in element inbetween SelectContainer and SelectItem', () => {
+ it('should start selection on element inbetween SelectContainer and SelectItem', () => {
------------------------------
In cypress/integration/dragging.spec.ts
<#167 (comment)>
:
> + .shouldSelect([1])
+ .getSelectBox()
+ .then(shouldBeInvisible)
+ .***@***.***')
+ .dispatch('mouseup');
+ });
+ });
+ });
+
+ it('should start selection in element inbetween SelectContainer and SelectItem', () => {
+ disableDragOverItems().then(() => {
+ getDesktopExample().within(() => {
+ cy.get('mat-grid-list')
+ .as('end')
+ .scrollIntoView()
+ .wait(16)
Why are all these manual wait points necessary? Generally I'd say it's not
ideal especially because Cypress has automatic wait points built-in in
almost all APIs. Can you elaborate why these are needed?
------------------------------
In projects/ngx-drag-to-select/src/lib/select-container.component.ts
<#167 (comment)>
:
> @@ -680,4 +683,13 @@ export class SelectContainerComponent implements AfterViewInit, OnDestroy, After
return null;
}
+
+ private _isClickOutsideSelectableItem(element: EventTarget): boolean {
+ if (!(element instanceof HTMLElement)) return false;
⬇️ Suggested change
- if (!(element instanceof HTMLElement)) return false;
+ if (!(element instanceof HTMLElement)) {
+ return false;
+ }
------------------------------
In projects/ngx-drag-to-select/src/lib/select-container.component.ts
<#167 (comment)>
:
> @@ -680,4 +683,13 @@ export class SelectContainerComponent implements AfterViewInit, OnDestroy, After
return null;
}
+
+ private _isClickOutsideSelectableItem(element: EventTarget): boolean {
+ if (!(element instanceof HTMLElement)) return false;
+
+ if (element === this.host) return true;
Add a block here too.
------------------------------
In projects/ngx-drag-to-select/src/lib/select-container.component.ts
<#167 (comment)>
:
> @@ -680,4 +683,13 @@ export class SelectContainerComponent implements AfterViewInit, OnDestroy, After
return null;
}
+
+ private _isClickOutsideSelectableItem(element: EventTarget): boolean {
+ if (!(element instanceof HTMLElement)) return false;
+
+ if (element === this.host) return true;
+ if (this._selectableItemsNative.includes(element)) return false;
Newline before and add a block.
—
Reply to this email directly, view it on GitHub
<#167 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACI4ZRXXWFZCALI4H77LDWDZOSUILAVCNFSM6AAAAABLP6A5TGVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDEMBTGQZDCNZRGI>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
This commit fixes the issue of not being able to start a selection if mousedown occured on neither Host element nor select item element but inbetween. The issue occurs if `SelectItem`s are not direct children of the `SelectContainer`. Fixes d3lm#144
PhOder
force-pushed
the
fix/144-select-on-click-prevents-drag-box-between-items
branch
from
308a731
to
23c2665
Compare
|
Thanks for the review @d3lm I hope everything checks out okay now :) |
|
Hello @d3lm |
This commit fixes the issue of not being able to start a selection if mousedown occured on neither Host element nor select item element but inbetween. The issue occurs if
SelectItems are not direct children of theSelectContainer.Fixes #144