adfinis / ember-uikit

The ember implementation of UIkit
https://adfinis.github.io/ember-uikit/
MIT License
26 stars 13 forks source link

Add Sortable Component #24

Closed jayvarner closed 6 years ago

jayvarner commented 6 years ago

I made a component today for UIkit's sortable component. I still need to add some more tests and finish the template for the demo site. Before I go too far with the demo site, I'd like some feedback on my approach to adding the component specific events and attributes. I noticed the evented mixin, it didn't seem right to add component specific events there. I'm sure I didn't follow some of your style conventions.

Anyway, I'll make whatever style/design adjustments and work on the demo page if y'all are interested in a PR.

Here's the commit to my fork: https://github.com/jayvarner/ember-uikit/commit/c42851356e300b6667fa3ef576d66fd98da9fbf3

anehx commented 6 years ago

First of all, thank you for contributing! Since we can only work on this infrequently, we are really glad someone other than us is interested in a progress on this.

You were right not to use the evented mixin. Those are simply all ember events converted to our naming convention. I looked at your commit and added some comments - mostly just nitpicking about our conventions (https://github.com/adfinis-sygroup/ember-uikit/blob/master/CONTRIBUTING.md). If you don't understand something I wrote, feel free to ask.

We are really interested in a PR for this feature, so feel free to open it!

jayvarner commented 6 years ago

OK, I mostly have this wrapped up but I have questions about how y'all want to move forward.

  1. I set up the import for UIkit (instead of using /* global UIkit */) by basically using the method described here and here using a vendor/shims/uikit.js shim and importing it along with the node_modules/uikit/js/dist/uikit.js file. It works, but I'm not 100% this is still the correct method. If it is, it seems I should open a separate issue/PR for that.

  2. I haven't finished the demo page because I'm running into some issues with updating the options. The sortable component has to be reinitialized for the new options to take effect. I first tried to update the component using UIkit's component.$update() method, but that didn't work:

    
    didInsertElement() {
    set(this, 'ukSortable', UIkit.sortable(this.element);
    },

didUpdate() { get(this, 'ukSortable').$update(); }


I tried various other ways of calling `update()` with no success. However, this works:

~~~javascript
didInsertElement() {
  set(this, 'ukSortable', UIkit.sortable(this.element);
},

willUpdate() {
  get(this, 'ukSortable').$destroy();
},

didUpdate() {
  set(this, 'ukSortable', UIkit.sortable(this.element));
}

I don't feel good about that. Maybe y'all have better ideas.

anehx commented 6 years ago

I set up the import for UIkit (instead of using / global UIkit /) by basically using the method described here and here using a vendor/shims/uikit.js shim and importing it along with the node_modules/uikit/js/dist/uikit.js file. It works, but I'm not 100% this is still the correct method. If it is, it seems I should open a separate issue/PR for that.

I think this method you used is still best practice unless you have a named or an anonymous AMD export in your external lib (https://ember-cli.com/user-guide/#standard-anonymous-amd-asset). As far as I remember, I tried using this but failed because UIkit does some other export before the AMD. So this is OK!

I haven't finished the demo page because I'm running into some issues with updating the options. The sortable component has to be reinitialized for the new options to take effect. I first tried to update the component using UIkit's component.$update() method, but that didn't work:

I had the same problem with the uk-tab component. I couldn't find a fast and clean solution, so I just disabled interaction out of laziness.. :wink: However, this is something we need to find a good solution which applies for all components.

I don't feel good about that. Maybe y'all have better ideas.

Me neither, sadly I don't have a better idea.. UIkit says they can handle DOM manipulations but obviously they can't handle updating properties (https://getuikit.com/docs/javascript#uikit-and-reactive-javascript-frameworks).

I may be able to invest some time in this too later this week.