-
Notifications
You must be signed in to change notification settings - Fork 36
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
Add Element Mixin #86
base: master
Are you sure you want to change the base?
Conversation
Also should add tests to this branch before this is included in a release, just in the process of converting the old tests, but that can come after this is merged in a new PR. Currently having some issues with using Skate's Jest environment package to enable using |
Oh other note - should we just have all mixins accessible at EDIT: Actually probably not, because we're compiling as a UMD (accessible at |
To the last point: yes, once they (and elements) are JS. Then there won't be any reason for them to be UMDs either. But for now yeah separate files and used as globals is the only way. |
Wait sorry just saw this is a JS file now - it should be an HTML import (for deduping), and consumed as a global, so that components themselves don't require build steps just to function. Again, once everything is JS then bundlers / ES modules take care of it. Until then it's not worth requiring a bundler buildstep on components just for this. |
And yes, we should definitely change the names of those hooks. I still vote for just |
@seaneking would you be able to setup the build for that? Been awhile since I've setup a gulpfile, particularly not with the new version. We'll need that to pipe it into HTML I believe. Unless we just use rollup and literally concat inside a |
Yeah don't see why we'd need gulp just to put it in a HTML file, just bundle it and manually write to a HTML file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Components are going to be JS now, so this can just export an ES6 module as-is. Still want to change the names of those callbacks though
Another thing to consider - if we ship UMD definitions of elements (for |
So not quite sure I fully understand, as deduping won't be affected by putting into global namespace. But
I think we should do this. Standard pattern for ES / UMD exports for all entry points we have. For UMDs that also means following a standard naming convention. Will make a note on #74 as more relevant there than here. |
Wires crossed, I didn't mean deduping resources in the network sense, I meant that you wouldn't have to pull and rebundle the mixin for every compiled UMD element definition. UMDs as a general pattern for Simpla core sounds good though 👍 Right now the dist files we have are in a bit of a limbo between the two |
Addresses #79
Adds the new ES6 SimplaElement mixin, converted from the ES5 Polymer Behavior at https://github.com/simplaelements/simpla-element-behavior to be a generic mixin for v1 Web Components.
Usage
Major Changes
simplaConfig
property rather than a factory functionupdateFromSimpla
andupdateSimplaBuffer
methods rather than declaringsetCallback
/getCallback
methods in the config, if they want to override the set / get behaviour e.g.May need to change the public names it adds - current choices were just from the private names implemented in the old behavior
Also tweaks how readonly / editable interact with one another, essentally the same behaviour I believe, but a little bit more robust