Zodiase / meteor-mdl

Material Design Lite package for Meteor
https://atmospherejs.com/zodiase/mdl
Apache License 2.0
53 stars 5 forks source link

Layout component re-rendering #30

Open ramijarrar opened 8 years ago

ramijarrar commented 8 years ago

Changing routes breaks the auto-upgrading mechanism at the moment, something similar to the work done for iron router would probably solve this.

ramijarrar commented 8 years ago

So it looks like the iron router patcher has been depreciated which probably means this is issue not so simple. I'll post back with more details soon.

ramijarrar commented 8 years ago

Alright - so to be more specific here, the issue I am having is with the drawer button not rendering after a page change. A refresh makes the issue go away but manually upgrading the components doesn't (so I don't think it is related to the component handler).

Here is the trace of error in the browser console:

image

Which maps to this line of code in material.js

@Zodiase Any thoughts?

Zodiase commented 8 years ago

The previously called "blaze patcher" which adds support to iron router is renamed to "autoUpgrade" with slightly changed interface in v1.1.0, though the default behavior is not changed. So if you were not using MDl.envConfig.patchers.blaze.setUpgradeStyle('fullUpgrade' | 'mutationOnly' | 'none'), it should not make any difference. But if you were, then setUpgradeStyle is moved to MDl.autoUpgrade.setUpgradeStyle.

At the moment from the error it looks to me the drawer is missing. But I don't see why that would happen. I just tested one of my apps using flow router with mdl v1.1.0 and didn't see anything abnormal. Do you have an example to reproduce the error?

Zodiase commented 8 years ago

In your app, is the drawer not persistent between routes?

ramijarrar commented 8 years ago

The drawer is definitely persistent between routes, but the topbar content changes.

ramijarrar commented 8 years ago

I'd be willing to make a repro, but just want to make sure that I am not missing anything obvious here.

Zodiase commented 8 years ago

MeteorPad does not have the latest packages so I can't show you the test I just made through there. But I put the major files in this gist: https://gist.github.com/Zodiase/12a44b4e023190d3a36c

ramijarrar commented 8 years ago

The drawer isn't being re-rendered or removed between routes in my case, only the topbar. Do you think that could be causing the issues? (The button exists in the topbar so it would make sense)

Zodiase commented 8 years ago

You are removing <header class="mdl-layout__header"> in some routes?

ramijarrar commented 8 years ago

It is always there, but is re-rendered.

Zodiase commented 8 years ago

So to me that sounds like you have something like this:

<div class="mdl-layout mdl-js-layout">
    {{header}}
    <div class="mdl-layout__drawer">{{drawerContent}}</div>
</div>

Which has a persistent drawer, but the header may change and gets re-rendered?

ramijarrar commented 8 years ago

The drawer is static (i.e drawer content does not change) but other than that essentially yes.

Zodiase commented 8 years ago

A funny thought of mine: you could actually create a static html page using MDL and simulate the layout changes with JavaScript and see how that plays out, without using Meteor at all.

ramijarrar commented 8 years ago

Well since we know it is working after a refresh, there is clearly some operation performed after the page has loaded which we need to re-trigger.

Zodiase commented 8 years ago

Back to the issue here.. What you described doesn't sound good.. You should never change the layout components once the layout is upgraded. Mainly because currently it's still impossible to "downgrade" a layout in MDL. More in detail: it's fine to have

<div class="mdl-layout mdl-js-layout">
    <header class="mdl-layout__header">{{headerContent}}</header>
    <div class="mdl-layout__drawer">{{drawerContent}}</div>
</div>

and change whatever you want the headerContent to be, that is assuming you are not using layout tabs (which gets really messy).

Zodiase commented 8 years ago

If you have this

<div class="mdl-layout mdl-js-layout">
    {{header}}
    <div class="mdl-layout__drawer">{{drawerContent}}</div>
</div>

and change header, what happens is {{header}} gets re-rendered, but not <div class="mdl-layout mdl-js-layout">. And that <div class="mdl-layout mdl-js-layout"> guy has already been flagged by MDL as "upgraded" so even though the autoUpgrader tries to upgrade it again, that does absolutely nothing.

Zodiase commented 8 years ago

And the result is the layout lose track of its head(er)...

ramijarrar commented 8 years ago

Is there a way to manually refresh this?

Zodiase commented 8 years ago

If you mean forcing <div class="mdl-layout mdl-js-layout"> to get re-rendered so it could be re-upgraded, no, not to my knowledge. Blaze seems to be doing a very good job at minimizing what to re-render. Even changing an attribute of an element only causes that attribute to be changed in DOM, but the element is not completely re-rendered.

Zodiase commented 8 years ago

BTW I'm working on implementing MDL with React, with the goal of eliminating this stupid upgrade thing... In other words, when that's done, MDL layout would be so much easier to use.

Zodiase commented 8 years ago

If you want to hide the header conditionally, either

ramijarrar commented 8 years ago

I'm not trying to hide the header, but I actually load the page content along with the header, so something like this:

<template name="samplePage">
    {{!-- Topbar --}}
    <header class="mdl-layout__header">
    </header>

    {{!-- Content --}}
    <main class="mdl-layout__content"></main>
</template>

And then in the layout:

 <div class="mdl-layout mdl-layout--fixed-drawer mdl-layout--fixed-header mdl-js-layout">
    {{> drawer}}

    {{> Template.dynamic template=page}}
</div>
ramijarrar commented 8 years ago

I can refactor this, but not sure how that would scale for other layouts (for example, we will be using tabs very soon for a settings page)

Zodiase commented 8 years ago

The layout basically has to stay like this

<div class="mdl-layout mdl-js-layout">
    <header class="mdl-layout__header">{{> Template.dynamic template=header}}</header>
    <div class="mdl-layout__drawer">{{> Template.dynamic template=drawer}}</div>
    <main class="mdl-layout__content">{{> Template.dynamic template=content}}</main>
</div>

A better version would be

<div class="mdl-layout mdl-js-layout">
    <header class="mdl-layout__header">{{> Template.dynamic template=header.template data=header.data}}</header>
    <div class="mdl-layout__drawer">{{> Template.dynamic template=drawer.template data=drawer.data}}</div>
    <main class="mdl-layout__content">{{> Template.dynamic template=content.template data=content.data}}</main>
</div>

so you can easily specify data context in the router.

Oh tabs... The ugly tabs... You know that the tabs are upgraded along with the layout. To me that basically means, if you want one route with tabs, the other not, then the only workable solution is to have two different layouts.

ramijarrar commented 8 years ago

Btw - regarding the React implementation, what does that mean for Blaze users?

Zodiase commented 8 years ago

For one thing, Meteor 1.3 is going to switch heavily to React, leaving Blaze behind... I'm not saying as a Blaze user I'm happy about that.

I think blaze and react can somewhat work together, though I have not tested.

I'll keep both packages updated, since to me prototyping with Blaze is still far more easier. And the effort in maintaining this package is nothing compared to the React version; I have to come up with new implementations for each component, and likely for each update of MDL.

Zodiase commented 8 years ago

The react wrapped versions of mdl on atmosphere right now almost all just use React to render the bare templates and ask the component handler to upgrade them. So that means very little effort in maintaining the package, but also means MDL's still very ugly to use with Meteor. What my mdl-react project is aiming to achieve is to get rid of the component handler, and implement all upgrade/downgrade logic inside each React component.

ramijarrar commented 8 years ago

Any reason (besides forwards-compatibility) that you have decided to use React for this? In other words are there certain limitations of Blaze at play here?

Zodiase commented 8 years ago

Well like the layout issue we were just talking about here, for one.

ramijarrar commented 8 years ago

What I mean to say is that you are describing an implementation of the components where the upgrade/downgrade logic is moved to the template level, and my question is whether or not that would actually require React components (vs Blaze) or if the decision is just based on forwards-compatibility.

Zodiase commented 8 years ago

And the auto upgrade mechanism ultimately is not going to be efficient in large apps where many DOM changes could happen at once.

Zodiase commented 8 years ago

The implementation depends on the React lifecycle events. Blaze's lifecycle hooks are not enough.

Zodiase commented 8 years ago

For example, Blaze only has onRendered, which happens only once. After that it's impossible to know when the template has been modified. And that's also why the auto upgrade mechanism has to rely on listening mutations on the whole page.

Zodiase commented 8 years ago

You are welcome to watch the progress on https://github.com/Zodiase/meteor-mdl-react and see if the implementations are portable to Blaze.

ramijarrar commented 8 years ago

Reflecting back on our discussion last night I'm not actually sure things like tabs will be a problem going forward since most people will want to control the rendering themselves anyways. This doesn't necessarily mean we have to build them from scratch since the styling for the tabs can most likely be used without any dependency on the js/component handler.

And regarding the suggestions you made concerning the separation of dynamic page/header content - this was something I was planning to do in the future anyways for more flexibility over layout. So really none of the issues we have uncovered here are necessarily prohibiting.

Also, considering the lifetime of the current architecture of the library (with a full rewrite of the architecture planned for 2.x) these "catches" will probably clean themselves up soon without any extra work from our end.

ramijarrar commented 8 years ago

I think this issue can be left open for now, in case someone else has any bright ideas or stumbles upon similar roadblocks in the library.

Zodiase commented 8 years ago

The issue with layout tabs is, when upgrading the layout, component handler will try to locate the tab bar with ".mdl-layout__tab-bar".

For all components, MDL locates the components with something like "mdl-js-button" for upgrading. I believe this was designed to allow you use the styling with "mdl-button" and prevent the element from being upgraded by leaving out the "js" counterpart.

However for layout that isn't the case; ".mdl-layout__tab-bar" contains critical styling and also is used to locate the tab bar. And once located, all of the rest of the tab upgrade will happen. At which point if anything seems wrong to MDL, errors will occur.

Working around that logic, I can think of a safe strategy is to add the tab bar only after the upgrade so MDL won't know it's there.

If the tab bar was present during the upgrading, then you can't modify the tabs without getting errors.