vpusher / paper-tree

Browsable tree of nodes with expandable/collapsible capabilities and actions menu
MIT License
27 stars 24 forks source link

Lazy loading events and mixins #3

Closed platoscave closed 7 years ago

platoscave commented 7 years ago

This branch contains features that facilitate lazy loading Including in paper-tree-node:

New files:

Chris

platoscave commented 7 years ago

@vpusher The problem with this approach is, it's not the icon we want to (un-)set to loading, it's the preicon. This would involve updating the class of the node from the 'toggle' event in the host. We would also have to inject the loading class from the host with one of those weird '>' notations. Any thoughts on how to achieve this?

While I agree with your philosophy, I don't necessarily agree that adding a loading state detracts from "dumbness". open can be thought of as having four states: expanded, collapsed, loading, none. This will work in any world. ...So another approach might be to change open from Boolean to something that can hold four states.

Lazy loading is especially important in enterprise applications where we're dealing with large amounts of data which we don't want to download all at once. This is where paper-tree will come into its own. It makes sense to provide minimal support for this out-of-the-box.

vpusher commented 7 years ago

I know how important it is to lazy load data, and how much you need it in some case. The thing is some will want to update the pre-icon, the others will want to update the icon and perhaps some others will want a loading text message (or both, or the three). That is to say the way the loading feedback is rendered is under the end developer responsibility. I don't want to take any opinion on this. This is a view that is bound to a model and it provides some hooks/events to implement you own logic.

To resume, to solve your issue the component needs the followings openings/hooks:

[x] know when children are toggled: a toggle event is needed. [ ] be able to change pre-icon: a mixin would be a good answer for that. [x] be able to change icon: you can actually do it editing the node's data.icon [x] be able to change text: you can actually do it editing the node's data.name

platoscave commented 7 years ago

@vpusher I've adopted your recommendations in the latest push. In order to enable changing the pre-icon I've add the --loading-icon-theme that can serve as a mixin to paper-tree-node.

The lazyload-demo is now much simpler and works fine, all except for applying the 'loading' class to the pre-icon span. The 'loading' class needs to be applied shortly after the node is created due to a data update. These spans are really hard to find. I thought about searching the dom tree but this is generally discouraged because "It requires the consumer to be aware about element's internals. Web component should only be used with properties/attributes and events".

What would be a good way to get access the pre-icon span so that we can modify it's class?

vpusher commented 7 years ago

@platoscave what about my last commit ? does it fit your needs ?

platoscave commented 7 years ago

We have to add target.loaded = true; to the demo's, Apart from that, that it works like a charm.

I'm also testing --paper-tree-icon-theme. It turns out that iron-icon doen't provide much in the way of mixins to do something with the icon, other than size and color. I found, what I think might be a better way to modify the icon:

setNodeIcon: function (node, icon) {
     var ironIcon = node.$$('iron-icon');
     ironIcon.set('src', icon);
 },

The only problem here is the issue which I mentioned earlier: iron-icon icon takes precedence over src contrary to docs #78. I'd have to hack my way around this as long as it hasn't been resolved. If this is an acceptable way of working we might not need --paper-tree-icon-theme.

Note: I had to add src/ to background:url(\'src/loading.svg\');\ in setLoadingState. I'm not sure if something similar is needed in your demo's, as I'm not running the code directly, but rather from my custom element.

How does one go about running the demo's, given that there is no bower-components context?

vpusher commented 7 years ago

@platoscave do you run it with polymer serve ?

benbenbenbenbenben commented 7 years ago

In my opinion the solution here is overly complicated. You can lazy render simply by using dom-if in a template on children. If a node isn't visible it isn't rendered (yet).

4

vpusher commented 7 years ago

@benbenbenbenbenben here we are mostly dealing with customizing the UI through openings/hooks such as events and mixins (to fit the use case of @platoscave). Data lazy loading is totally in charged of the end developer. That's the first point. The second point is about lazy render the tree, and that is the purpose of your PR. Tank you for that, I will consider it, merge it and then release a new version of the component with those 2 enhancements (PRs).

vpusher commented 7 years ago

@platoscave I am merging this one. Most of your use cases are covered now, and the current implementation seems generic enough to me. Please address a new PR for the icon src issue if you really need it.

benbenbenbenbenben commented 7 years ago

@vpusher okay thanks for this

vpusher commented 7 years ago

@platoscave Released in v1.0.3