Skip to content

attempt to add loader to playlist#210

Merged
bcomnes merged 3 commits intomasterfrom
loader
Jul 5, 2017
Merged

attempt to add loader to playlist#210
bcomnes merged 3 commits intomasterfrom
loader

Conversation

@ungoldman
Copy link
Copy Markdown
Member

@bcomnes this PR shows the problem, which is still persisting for me. The cog spins when loading, but the playlist remains unchanged.

If you switch the line in Playlist._render from

if (this._loading) return loader()

to

return loader()

you get

screen shot 2017-07-02 at 10 43 51 pm

so I'm guessing this is an _update method issue?

@ungoldman ungoldman requested a review from bcomnes July 3, 2017 06:00
@bcomnes
Copy link
Copy Markdown
Contributor

bcomnes commented Jul 3, 2017

Ill take a look. Sounds like maybe an _update issue.

@bcomnes
Copy link
Copy Markdown
Contributor

bcomnes commented Jul 5, 2017

I made it work. For some reason having the root node types differ confused nanomnorph, resulting in a noop morph. ¯\_(ツ)_/¯ Maybe an upstream bug with nanomnorph. I'll ask around.


module.exports = function loader () {
return html`
<section class="${styles.loading}">
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Changing this from div to section, to match the other main root node, it works. Probably a nanomnorph bug.

@bcomnes bcomnes merged commit 26c9428 into master Jul 5, 2017
@bcomnes bcomnes deleted the loader branch July 5, 2017 03:05
@ungoldman
Copy link
Copy Markdown
Member Author

Just read your comments above -- still don't really get why the root node types differing would matter. I'll try to figure it out tomorrow.

@bcomnes
Copy link
Copy Markdown
Contributor

bcomnes commented Jul 5, 2017

It shouldn't matter, I don't think. I think its a bug in nanomnorph. I'm guessing its related to the fact these have the same ID but different root nodes + the child reordering algorithm that was just introduced.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants