Paper tabs

[bjornharrtell]

Continuation of PR #578, rebased and fixed jscs errors.

[bjornharrtell]

@miguelcobain, I think this is ready for review and possibly inclusion in 1.0.0. If the issue with event namespacing mentioned in #578 is serious enough to be blocking I'll be glad to attempt to fix it.

[miguelcobain]

I don't think that "tab three" ellipsis should he happening, right?

[panthony]

I did list some issues on my PR:

• paper-tabs: resize binding is never removed and should be namespaced
• paper-tabs: height is not updated when content is updated with dynamicHeight=true
• paper-tabs resize callback may fail with Cannot read property 'outerWidth' of undefined (callback on destroyed object)
• missing unit tests
• paper-tab: the label may get truncated by the fixed width set to the component

I think they are all pretty important for a stable release.

[bjornharrtell]

Thanks @panthony. I've fixed the next/prev pagination buttons to properly display icons and disabled the fixed width for tabs so that labels are no longer truncated. I haven't found any adverse effects.

[panthony]

@bjornharrtell The adverse effect will be that tabs won't have the same width although according to material design specs: Fixed tabs have equal width (docs)

The problem is that there is no demo in the dummy app and no unit/integration tests so it's hard to modify and be sure they are no side effects.

That's why I tried to add some tests first to check the current behavior, and modify the code afterward.

Edit:

Apparently 'scrollable tabs' do not need to be the same width though.

[bjornharrtell]

@panthony agreed that tests first is the right approach. I just need to get more familiar with ember-paper and this branch before I'll be able to do that. I did have a hard time reproducing the overflow problem though, it happens just 1 out of 5 reloads with no changes to source/templates. I guess there might a render timing issue that affects the width calculation.

[panthony]

@bjornharrtell I may be able to take another shot at it this week end since i'll be stuck in a train for 6-7 hours. I could try to spend some of it writing tests and/or exemple in the dummy app.

[bjornharrtell]

Would be great, keep me posted. :)

I'm currently debugging a situation where I get Assertion Failed: You modified isActive twice on <client@component:paper-tab-content::ember564> in a single render..

[panthony]

Probably because the tab registers themselves to their parent in a 'afterRender' callback.

The parent display with isActive to whatever, then a tab render itself, then it register itself in afterRender which modify the tabs on the parent, which probably make the 'selected' state recompute that retrigger le 'isActive' props...

Maybe the components should register/unregister themselves upon init/destroy.

Then way the 'didInsertElement' or 'afterRender' events to mesure/update styles.

[bjornharrtell]

Was able to write a test case for the last discussed error.

[miguelcobain]

ember-composability-tools provides some tools for these scenarios. It is already being used in paper-form.
It registers children to parent on init, so it may allow us overcome this issue.

[bjornharrtell]

Thanks @miguelcobain will look into it. It seems this case only produced deprecation warnings before 2.10, this issue explains emberjs/ember.js#13948.

[miguelcobain]

Correct. A deprecation turned into an assertion.

[bjornharrtell]

Not getting anywhere. I find it hard to understand the home grown composability in this branch, which would be a requirement to be able to replace it. I can't say that I fully understand ember-composability-tools either yet... but it does look more clean.

[bjornharrtell]

Seems some of the practices in the code is in need of refactoring - some guidance here https://github.com/GavinJoyce/backtracking.

[bjornharrtell]

While I have a long way to be able to refactor, I did find a fix for the re-render issue.

[panthony]

Currently working on it toi, added some exemples in dummy app but i feel like stretchTab and alignTabs settings should be merged in à fixedTabs bool to better follow MD specs but hard to keep and only 2 cases to handle

[panthony]

My bad tabs can be centered in MD specs without being fixed (all width and equal width)

[bjornharrtell]

I've rebased this against #578 and master.

[bjornharrtell]

I think the remaining known issues now are:

• paper-tabs: height is not updated when content is updated with dynamicHeight=true
• paper-tab: the label may get truncated by the fixed width set to the component

[bjornharrtell]

I cannot reproduce the label truncation problem in Firefox or Chrome with devtools enabled, won't be easy to track down... only intermittent in plain Chrome. I suspect rendering order/timing. On initial render I get 9 widthStyle computes for paper-tab, which does not feel good either. From what I can tell a faulty width is 6 pixels smaller than a correct width calculation.

[bjornharrtell]

Got it! :) The issue was that the tab width could be fractional depending on tab label text rendering specifics and offsetWidth returns only integer value which might be rounded down resulting in truncation.

[bjornharrtell]

About the issue "height is not updated when content is updated with dynamicHeight=true" I'm not sure there is anything sane that can be done except perhaps a manual trigger. I don't feel it should be a blocking issue for merge and in that case this PR is now ready for review again.

When possible will you have a look at this again @miguelcobain?

[panthony]

@bjornharrtell When I had the issue with the truncated tabs it was not always of the same offset, and more often than not much more than few pixels. Not sure why we need to set the width on tab that should adapt to their content in the first place.

There is also another "issue" is that stretchedTabs does not work. They should take the full canvas width and split equally but they are not.

For the dynamicHeight I don't really see a nice fix either.

[bjornharrtell]

Agreed, the code does to much low level work and the heavy use of observers isn't desirable. However, after my fix at least I do not get any truncation any more.

[bjornharrtell]

This is obsoleted by #578.