Custom drag placeholder

[Devin-Harris]

Merge request for Custom drag placeholder functionality described in the TODO features of the project.

Change list includes using a TemplateRef for the placeholder div, then instead of dynamically appending and removing it from the dom, dynamically adjusting its display style from 'none' to 'block' so the template can have a ngTemplateOutlet somewhere on the dom at all times. Also created a new route for the demo app to showcase changing the custom placeholder dynamically and added some documentation into the readme.

[llorenspujol]

Hi @Devin-Harris , thank your for you pull request!

The PR is nice, it lets customize the placeholder, and is far better than what we have now, but it would be also better if we can customize each ng-template for each ktd-grid-item. One common use case is reproduce the content inside ktd-grid-item in the placeholder, right now with this implementation is quite difficult to do so. For example, in the 'real-life-example', we may want to recreate each chart on the placeholder with some opacity. For reference, final solution of custom placeholder may be quite similar to the one's in angular material: https://material.angular.io/cdk/drag-drop/examples#cdk-drag-drop-custom-placeholder

Final API may be something like:

<ktd-grid [cols]="cols"
          [rowHeight]="rowHeight"
          [layout]="layout"
          (layoutUpdated)="onLayoutUpdated($event)">
    <ktd-grid-item *ngFor="let item of layout; trackBy:trackById" [id]="item.id">
         <div class="grid-item-content">{{item.id}}</div>
         <ng-template ktdGriItemPlaceholder let-item='item' [data]="{item: item}">
               <div class="grid-item-content">{{item.id}}</div>
         </ng-template>
    </ktd-grid-item>
</ktd-grid>

Note that we want to pass custom data to ng-template, to do it we may want to use a custom directive.

If you have other suggestions we can discuss them. This solution is pretty solid overall, but is also not as simple as I would like to be, so I am open to discuss alternatives if there are.

[Devin-Harris]

Hi @Devin-Harris , thank your for you pull request!

The PR is nice, it lets customize the placeholder, and is far better than what we have now, but it would be also better if we can customize each ng-template for each ktd-grid-item. One common use case is reproduce the content inside ktd-grid-item in the placeholder, right now with this implementation is quite difficult to do so. For example, in the 'real-life-example', we may want to recreate each chart on the placeholder with some opacity. For reference, final solution of custom placeholder may be quite similar to the one's in angular material: https://material.angular.io/cdk/drag-drop/examples#cdk-drag-drop-custom-placeholder

Final API may be something like:

<ktd-grid [cols]="cols"
          [rowHeight]="rowHeight"
          [layout]="layout"
          (layoutUpdated)="onLayoutUpdated($event)">
    <ktd-grid-item *ngFor="let item of layout; trackBy:trackById" [id]="item.id">
         <div class="grid-item-content">{{item.id}}</div>
         <ng-template ktdGriItemPlaceholder let-item='item' [data]="{item: item}">
               <div class="grid-item-content">{{item.id}}</div>
         </ng-template>
    </ktd-grid-item>
</ktd-grid>

Note that we want to pass custom data to ng-template, to do it we may want to use a custom directive.

If you have other suggestions we can discuss them. This solution is pretty solid overall, but is also not as simple as I would like to be, so I am open to discuss alternatives if there are.

Yes I like the idea of having each grid-item have their own custom placeholder as well. I originally did not do it this way as there is only ever one placeholder div that is a sibling of the grid-items, but I could mess around with reimplementing this from the grid-item level.

[llorenspujol]

@Devin-Harris Yes, I know. You implemented well the custom placeholder, but in this case I think we want the item-customizable solution. If you want to do it great, if not, I would do myself in some time. It is not a priority for the lib right now, but is really nice to have feature, and would be added and versioned as soon as it is done.

If you are gonna do it, and have some questions, we can talk about it on Discord or so. This way we both may lose less time in the process of adding this feature. Said that, the solution I imagine is quite close angular material one's. But I repeat, If you have implementation doubts, like functionality or whatever, we can talk about it.

[Devin-Harris]

@llorenspujol No worries I don't mind taking a stab at it. I've reworked it slightly so custom placeholders can be specific/unique to each grid item. Not sure if a directive is needed for custom data since they have the item information from the containing ktd-grid-item component, but will continue investigating if that would be beneficial

[Devin-Harris]

I added some comments about the current solution. I think we should really create the placeholder 'on-demand', sorry again for not pointing that out firstly. About the final solution, ktd-grid-item code should be nearly the same as before, just with the placeholder ContentChild. That placeholder, if provided, should be created when drag starts, if not, default placeholder is created. If you have any doubts tell me, and thanks for your work!

I see your point about the benefit to dynamically appending the placeholder as the drag occurs. My initial issues that prompted me to develop around this feature was that it was difficult for me to find where the red color was coming from on the project I am using this for, because as soon as I release the drag to inspect the console the div disappears. Obviously I eventually found it so its probably not a big issue and definitely not big enough to introduce potential efficiency issues with more complex placeholders. I've reverted some of those changes and reworked it to pull in the directives elementref and append that to the dynamically generated ktd-grid-item-placeholder div. Let me know if this is more inline with what you were looking for here 😄 .

[llorenspujol]

@Devin-Harris I added a commit, with the dynamic creation of the placeholder. It is still not finished, there are some issues to solve. The fact that the directive should support an Input() data is because performance issues, this way angular does not have to bind anything to any ng-template AFAIK. Afterwards is optional to use, but is nice to have.

Things that are missing:

• Real life example should reproduce the same element in the grid as placeholder. This is kind a buggy right now, sizes are not correctly passed to ngx-charts, we need to check why and solve it.
• I am not sure if let the 0.6 opacity for custom placeholders, I think it should go away.

Maybe later or tomorrow I can put more time into solve those issues. But If you have some spare time and want to solve it or give feedback would be nice. We can maybe make a release with the gap and custom placeholders this week

[Devin-Harris]

@Devin-Harris I added a commit, with the dynamic creation of the placeholder. It is still not finished, there are some issues to solve. The fact that the directive should support an Input() data is because performance issues, this way angular does not have to bind anything to any ng-template AFAIK. Afterwards is optional to use, but is nice to have.

Things that are missing:

• Real life example should reproduce the same element in the grid as placeholder. This is kind a buggy right now, sizes are not correctly passed to ngx-charts, we need to check why and solve it.
• I am not sure if let the 0.6 opacity for custom placeholders, I think it should go away.

Maybe later or tomorrow I can put more time into solve those issues. But If you have some spare time and want to solve it or give feedback would be nice. We can maybe make a release with the gap and custom placeholders this week

Fair enough. I don't know enough about those binding performance issues on ng-templates but it sounds plausible so I think the rework to use that is fine. For the opacity I definitely don't think it should be applied to the custom placeholders. My goal that prompted me to work on this was to not have to do styling overriding from outside the KTD scope which I still would if opacity affects non default placeholders. I think just moving that opacity: 0.6 style from the .ktd-grid-item-placeholder class to the &-default block would resolve this. Also I see you removed the Default option from the custom placeholders example... I do like the none option, but I still think it would be beneficial to see the Default one in case someone wants to know why they have a low opacity red block as their placeholder. Alternatively we could make the None the default placeholder, but I don't think that's the better option. As far as the ngx charts stuff, I don't really have any good ideas. We could try to trigger change detection every time we update the width/height on the placeholder, but that sounds inefficient. I also thought maybe we could emit an event when the placeholder resizes and let the user handle what to do with that event. I like this idea better, but would be interested in hearing your thoughts. Finally I think we need to update the readme since we are requiring the ktdGridItemPlaceholder directive to be a template. I think that covers everything I noticed/thought, but of course their may be gaps in my understanding of the project so feel free to disregard anything that may not make sense hahaha.

[llorenspujol]

@Devin-Harris I added a commit, with the dynamic creation of the placeholder. It is still not finished, there are some issues to solve. The fact that the directive should support an Input() data is because performance issues, this way angular does not have to bind anything to any ng-template AFAIK. Afterwards is optional to use, but is nice to have. Things that are missing:

• Real life example should reproduce the same element in the grid as placeholder. This is kind a buggy right now, sizes are not correctly passed to ngx-charts, we need to check why and solve it.
• I am not sure if let the 0.6 opacity for custom placeholders, I think it should go away.

Maybe later or tomorrow I can put more time into solve those issues. But If you have some spare time and want to solve it or give feedback would be nice. We can maybe make a release with the gap and custom placeholders this week

Fair enough. I don't know enough about those binding performance issues on ng-templates but it sounds plausible so I think the rework to use that is fine. For the opacity I definitely don't think it should be applied to the custom placeholders. My goal that prompted me to work on this was to not have to do styling overriding from outside the KTD scope which I still would if opacity affects non default placeholders. I think just moving that opacity: 0.6 style from the .ktd-grid-item-placeholder class to the &-default block would resolve this. Also I see you removed the Default option from the custom placeholders example... I do like the none option, but I still think it would be beneficial to see the Default one in case someone wants to know why they have a low opacity red block as their placeholder. Alternatively we could make the None the default placeholder, but I don't think that's the better option. As far as the ngx charts stuff, I don't really have any good ideas. We could try to trigger change detection every time we update the width/height on the placeholder, but that sounds inefficient. I also thought maybe we could emit an event when the placeholder resizes and let the user handle what to do with that event. I like this idea better, but would be interested in hearing your thoughts. Finally I think we need to update the readme since we are requiring the ktdGridItemPlaceholder directive to be a template. I think that covers everything I noticed/thought, but of course their may be gaps in my understanding of the project so feel free to disregard anything that may not make sense hahaha.

Yeah, what you say makes sense, I agree on all of it 👍 , README.md needs also to be updated, and I forgot 😆 . if you want to make those changes would be nice.

About the ngx-charts placeholder issue, we should be able to understand why they are not rendered correctly. It seems an ngx-charts issue, but we can not discard a ktd-grid issue. It is strange, since ngx-charts, if no view Input is passed they get the parent bounds, and the parent bounds are well defined... I should be missing something. Maybe is because the placeholder is not already in the dom? I don't know, but it should work without hacks. If we can't understand why is happening and solve it cleanly we can make a 'patch' for now. About patches, we can just execute the same code that gets executed when 'resize' event is fired. Seems that after a resize all works well, because layoutSizes have value.... either way is strange.

[Devin-Harris]

@llorenspujol Alrighty I added a couple things in 4 separate commits above. First I added the default option back into the custom-placeholders demo. Second I moved the opacity to the &-default block and fixed the opacity for the real-life-example charts to be lower opacity. Third I added logic for detecting when the placeholder has had a change to its width or height and then emit a placeholderResize event that is then handled on the parent of the ktd-grid component, in this case the real-life-example page. Interested to see if you think their is a better way to do this, but from my experience with ngx charts, their widths and heights aren't updated with the parent unless some sort of change detection is triggered manually, so I trigger that from the parent level to accomplish that. I don't think this is an unique issue to this project, rather an issue with ngx charts, but could be wrong so if you want to investigate this further by all means do so. And lastly I updated the readme.

Let me know if these changes are in line with what you were thinking 😄

[llorenspujol]

@llorenspujol Alrighty I added a couple things in 4 separate commits above. First I added the default option back into the custom-placeholders demo. Second I moved the opacity to the &-default block and fixed the opacity for the real-life-example charts to be lower opacity. Third I added logic for detecting when the placeholder has had a change to its width or height and then emit a placeholderResize event that is then handled on the parent of the ktd-grid component, in this case the real-life-example page. Interested to see if you think their is a better way to do this, but from my experience with ngx charts, their widths and heights aren't updated with the parent unless some sort of change detection is triggered manually, so I trigger that from the parent level to accomplish that. I don't think this is an unique issue to this project, rather an issue with ngx charts, but could be wrong so if you want to investigate this further by all means do so. And lastly I updated the readme.

Let me know if these changes are in line with what you were thinking 😄

Thanks for the work 👍 . It really makes sense to emit when an item is being resized and changes its bounds, since user may want to perform some action like resize. Basically I changed the event to be named gridItemResize to be more generic. I added some small changes too, but idea is the same. About the buggy ngx-charts, there was 2 things, first I was not setting the correct value for the 'view' Input, and secondly, this thing with the [data] and the directive, is not working dynamically🤣 (so I am not using it on real life example). Also I have moved the custom placeholder example to the playground one's, is just one selector, so I think it would be better there for now. If one day custom placeholders example is expanded, it may have its own page.

[llorenspujol]

@Devin-Harris Only things missing are squasing all those commits into 1, and rebasing the main branch afterwards. After that, it will be ready to be merged!. If you have some concerns about the final solution tell me, we can discuss about it, would be nice if you can test also. Thanks!

[Devin-Harris]

@llorenspujol Went through your changes and I agree with all your changes. I think the data you have passing up is a lot cleaner and useful. I updated the readme and squashed the commits so it should be good to go if you want to give it a once over and merge it 😄

[llorenspujol]

Nice @Devin-Harris ! I added some commits, basically for removing a duplicate function on playground example (due to merge). I think right now is ready to merge, so after you squash those commits I would merge it! 👏 . Great work 🤝!

[Devin-Harris]

@llorenspujol Annnnd squashed... Should be good to merge whenever you're ready 😁

[llorenspujol]

@Devin-Harris version 2.0.0 released! If you want a test out would be nice. Have a nice day!

[llorenspujol]

@Devin-Harris do you have some twitter account? I would tag you if so, since I normally notify it on twitter the new versions

[Devin-Harris]

@Devin-Harris do you have some twitter account? I would tag you if so, since I normally notify it on twitter the new versions

I don't have a Twitter but thank you for asking. Can't wait to update to the new release 😁😁