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.

Sign up for GitHub

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

Jump to bottom

feat(lib): add disabled state for select item #130

Merged
merged 7 commits into from
Sep 1, 2021
Merged

feat(lib): add disabled state for select item #130

merged 7 commits into from
Sep 1, 2021

Conversation

ennjin
Copy link
Contributor

@ennjin ennjin commented Aug 10, 2021

Closes #71

@ennjin
Copy link
Contributor Author

ennjin commented Aug 11, 2021

@d3lm Hey. Can you take a look at this, please?

d3lm
d3lm requested changes Aug 17, 2021
View reviewed changes
Copy link
Owner

@d3lm d3lm left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! Very much appreciated. Left a few small comments. Otherwise it looks good 👍

README.md Outdated Show resolved Hide resolved
projects/ngx-drag-to-select/src/lib/select-container.spec.ts Show resolved Hide resolved
src/app/app.component.html Outdated
@@ -22,6 +22,7 @@ <h3>
<mat-checkbox data-cy="selectOnDrag" [(ngModel)]="selectOnDrag">Select on Drag</mat-checkbox>
<mat-checkbox data-cy="dragOverItems" [(ngModel)]="dragOverItems">Drag Over Items</mat-checkbox>
<mat-checkbox data-cy="disable" [(ngModel)]="disable">Disable</mat-checkbox>
<mat-checkbox data-cy="useDisableCondition" [(ngModel)]="useDisableCondition">Disable condition (even)</mat-checkbox>
Copy link
Owner

Choose a reason for hiding this comment

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

For the text of the checkbox let's just say Disable even items.

src/app/app.component.html Outdated
@@ -22,6 +22,7 @@ <h3>
<mat-checkbox data-cy="selectOnDrag" [(ngModel)]="selectOnDrag">Select on Drag</mat-checkbox>
<mat-checkbox data-cy="dragOverItems" [(ngModel)]="dragOverItems">Drag Over Items</mat-checkbox>
<mat-checkbox data-cy="disable" [(ngModel)]="disable">Disable</mat-checkbox>
<mat-checkbox data-cy="useDisableCondition" [(ngModel)]="useDisableCondition">Disable condition (even)</mat-checkbox>
Copy link
Owner

Choose a reason for hiding this comment

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

Also could you change useDisableCondition to disableEvenItems

@ennjin
Copy link
Contributor Author

ennjin commented Aug 17, 2021

Hi. I've updated the notes above :)

@ennjin ennjin requested a review from d3lm August 17, 2021 14:51
@ennjin
Copy link
Contributor Author

ennjin commented Aug 26, 2021

@d3lm We're blocked by these changes, can you take a look at this as well?

@d3lm
Copy link
Owner

d3lm commented Aug 27, 2021

Please rebase the PR on latest master. I have fixed CI.

@ennjin ennjin force-pushed the feat(lib)--disabled-condition branch from 598ed7a to 9d83093 Compare August 27, 2021 09:14
@ennjin
Copy link
Contributor Author

ennjin commented Aug 27, 2021

@d3lm Done

@ennjin ennjin force-pushed the feat(lib)--disabled-condition branch from 9d83093 to ab74310 Compare August 30, 2021 06:37
@ennjin
Copy link
Contributor Author

ennjin commented Aug 30, 2021

There're latest changes from master branch.

d3lm
d3lm requested changes Aug 30, 2021
View reviewed changes
Copy link
Owner

@d3lm d3lm left a comment

Choose a reason for hiding this comment

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

This looks good. One last thing, could you also add an e2e test for this? 🙏

projects/ngx-drag-to-select/src/lib/select-item.directive.ts Outdated
Comment on lines 37 to 38
@HostBinding('class.dts-disabled')
@Input()
Copy link
Owner

Choose a reason for hiding this comment

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

It's a nitpick but can you please change the order here

Suggested change
@HostBinding('class.dts-disabled')
@Input()
@Input()
@HostBinding('class.dts-disabled')

@ennjin
Copy link
Contributor Author

ennjin commented Aug 30, 2021

Updated!

@ennjin ennjin requested a review from d3lm August 31, 2021 14:28
d3lm
d3lm requested changes Aug 31, 2021
View reviewed changes
Copy link
Owner

@d3lm d3lm left a comment

Choose a reason for hiding this comment

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

Awesome work! One last small thing.

cypress/integration/styling-and-classes.spec.ts Outdated Show resolved Hide resolved
@ennjin @d3lm
fix: update cypress/integration/styling-and-classes.spec.ts
e244620 
Co-authored-by: Dominic Elm <d3lm@users.noreply.github.com>
d3lm
d3lm approved these changes Sep 1, 2021
View reviewed changes
cypress/integration/styling-and-classes.spec.ts Outdated Show resolved Hide resolved
@d3lm d3lm merged commit ec1116e into d3lm:master Sep 1, 2021
@d3lm
Copy link
Owner

d3lm commented Sep 1, 2021

Merged! Thanks a lot for this PR 🙏

@ennjin
Copy link
Contributor Author

ennjin commented Sep 1, 2021

@d3lm Thanks! I'm wondering if could you bump the new changes to the NPM?

@ennjin ennjin deleted the feat(lib)--disabled-condition branch September 1, 2021 10:47
@d3lm
Copy link
Owner

d3lm commented Sep 1, 2021

Definitely. I am trying to push on the other PR too so I can cut a single release that includes both new features.

@d3lm
Copy link
Owner

d3lm commented Sep 1, 2021

It's this one #129.

@d3lm
Copy link
Owner

d3lm commented Sep 3, 2021

I am not hearing anything, so maybe we can cut a release already. Alternatively you could pick up the PR and we can get it over the finish line.

@ennjin
Copy link
Contributor Author

ennjin commented Sep 6, 2021

I am not hearing anything, so maybe we can cut a release already

Yep, it would be nice if we released it

@ennjin
Copy link
Contributor Author

ennjin commented Sep 8, 2021 •
edited

So, will you bump the new version?

UPD:
@d3lm ping

sam-eah pushed a commit to sam-eah/ngx-drag-to-select that referenced this pull request Sep 10, 2021
@ennjin @sam-eah
feat(lib): allow select items to be disabled (d3lm#130)
3ec194b
@d3lm
Copy link
Owner

d3lm commented Sep 20, 2021

It's now published. You can grab 4.2.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@d3lm d3lm d3lm approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Is there a way to programatically enable/disable an element as selectable?
2 participants
@ennjin @d3lm