[AppBar] waterfall behaviour #1321

igorbt · 2015-08-02T14:02:24Z

Implemented a type of AppBar position / behavior that basically transition AppBar from an initial height (maxHeight) to a minimum height while scrolling, with possibility of controlling transitions of AppBar children to create nice effects (check example in the docs and screens below).

I named it waterfall, because of a similarity with Polymer's paper-header-element waterfall mode.

Changed implementation of fixed position. Now AppCanvas don't set anymore position fixed on a child AppBar. Instead, AppBar should specify it's position (witch, by default is "fixed").

Also, AppBar now takes care by itself of creating a spacer div when in position fixed (or waterfall), so that there's no need of putting margins/paddings on the static contents that is below AppBar.

I needed a full-page example for demo in docs, this is why I made a new route for this.

igorbt · 2015-10-29T08:23:16Z

I published a demo of the Waterfall AppBar and here are updated docs for AppBar.

Please review this or at least put here a comment if you like it, otherwise I think there's no reason to update this 2 months old PR and I will likely just close it.

@oliviertassinari, don't you want to take a quick look here?

c0b41 · 2015-10-29T09:28:32Z

Nice works 👍

shaurya947 · 2015-11-10T19:35:46Z

I like this. @igorbt can you rebase?

@oliviertassinari take a look too. We can merge then.

oliviertassinari · 2015-11-10T20:27:04Z

src/app-canvas.jsx

@@ -16,26 +16,9 @@ let AppCanvas = React.createClass({
      WebkitFontSmoothing: 'antialiased',
    };

-    let newChildren = React.Children.map(this.props.children, (currentChild) => {
-      if (!currentChild) { // If undefined, skip it


Why did you remove this bloc?

Because AppBar itsefl takes care of it's position. There is a new prop called position.

pandaiolo · 2015-11-24T16:03:03Z

Cool feature !

shaurya947 · 2015-11-24T16:28:58Z

@igorbt still needs rebase..

igorbt · 2015-11-24T22:36:42Z

Thanks for comments. No time for rebase this week. I'll try next week.

alitaheri · 2015-12-10T10:44:37Z

@oliviertassinari This has fallen too far behind, I couldn't rebase it in a way that it would work. I think we should close this.

pandaiolo · 2015-12-10T10:49:23Z

Ahhh... too bad... I'd like to have a look at it, if a modified PR can fit, but I don't have time ATM. Anyone know how do I copy this branch into my local repo?

alitaheri · 2015-12-10T10:56:08Z

add https://github.com/igorbt/material-ui.git as a remote and checkout the waterfall-appbar from it.

I tried everything! I first rebased the commit into one. the n I tried doing rebase onto master! HELL STORM OF CONFLICTS! so no luck. then I tried to rebase release by release. it did the job, but ended up with a dysfunctional AppBar -_-

alitaheri · 2015-12-10T10:56:42Z

P.S. I can share my dysfunctional branch with you if you'd like to continue working on it.

pandaiolo · 2015-12-10T11:35:24Z

Yes please, so I can check both versions out ! Thanks

alitaheri · 2015-12-10T11:48:37Z

@pandaiolo Alright, do the following:

git add remote subjectix https://github.com/subjectix/material-ui.git
git fetch subjectix
git branch -av

Now you will see 3 branches on subjectix:

waterfall-original: this is the original branch, the work done by @igorbt
waterfall-rebased-onto-pr-base : this is the rebased version of @igorbt's work, still good and functional, but old, I simply squashed down his commits into one.
waterfall-rebased-onto-master: this is my dysfunctional rebase result. I rebased onto master step by step (tag by tag) but ended up with an AppBar that doesn't work well.

You are welcome to submit a PR if you can get this fixed.

igorbt · 2015-12-10T12:59:16Z

Sorry for late reply, but I have good news. I rebased and also adapted docs. It's not pushed yet but I'll do that later today after a last look at the code.

changed implementation for an optimized performance

igorbt · 2015-12-10T23:22:39Z

OK, I finally managed to rebase, improved a bit the feature and the docs and adapted the tests.

I also updated demo page and also here you can see how this feature is used in the docs home page.

@shaurya947 , @oliviertassinari, @ayhankuru, @pandaiolo, @subjectix can you please have a look?

newoga · 2015-12-11T04:54:11Z

@igorbt, this is cool! 👍

While I like the waterfall name, I noticed the spec calls this concept flexible space.

Also,

The app bar can scroll off-screen with the content and return when the user reverse scrolls.

The app bar can stay fixed at the top with content scrolling under it.

It'd be cool to support option 1 in this or a future pull request! I'd also be curious to hear how we can support differentiating these four blocks:

Status bar

Tool bar

Tab bar/search bar

Flexible space: space to accommodate a desired aspect ratio for images or extended app bars

c0b41 · 2015-12-11T05:51:24Z

@igorbt yeah so cool, i think merge need this a long time.

@newoga nice feature 👍

igorbt · 2015-12-11T06:54:05Z

@newoga , @ayhankuru thanks!

@newoga , thanks for link to the specs. I somehow missed this part of the Material Design spec and only saw this feature in the Polymer implementation (this is where I found the name "waterfall"). I wish I knew about this spec and do in "by the book" from the start. I definitely need behavior 1 for scrolling in my app, so I would like to invest some time in it. Maybe in a next iteration I could get AppBar closer to spec.

alitaheri · 2015-12-11T09:57:59Z

@igorbt Thanks for taking the time to do a rebase on this.

@newoga I like your proposals all the way. but this PR was long awaited, I think it's better to merge this now and add those feature in the next iteration. @oliviertassinari What do you think?

alitaheri · 2015-12-11T10:13:19Z

@igorbt Really nice work 👍 👍 It's beautiful 😍 I'review the code too, and when the review is done. we'll merge this 🎉

alitaheri · 2015-12-11T10:31:57Z

docs/src/app/app-routes.jsx

-    </Route>
+      <Redirect from="components" to="/components/appbar" />
+      <Route path="components" component={Components}>
+        <Route path="appbar" component={AppBar} />


please change this back to app-bar, this breaks navigation in the components section

oliviertassinari · 2015-12-11T12:17:58Z

src/app-bar.jsx

@@ -127,6 +159,7 @@ const AppBar = React.createClass({
      showMenuIconButton: true,
      title: '',
      zDepth: 1,
+      position: 'fixed',


What about using static as the default?
Should we really introduce this breaking change?

I agree with you. this should be 'static' by default

IMHO, main use case here is 'fixed', because, before, AppCanvas overrided it's position to fixed. So, putting it to 'static' would be more of a breaking change. But, of course it depends of how people are using it.
I can change it to 'static' if you still say so.

Hmmm you do have a point, @oliviertassinari What do you think? maybe just put a note in the release. I guess there are more people using AppCanvas than AppBar alone.

oliviertassinari · 2015-12-12T18:09:11Z

Having a look at all the posibility suggested by the specification.
I think that we should have a "layout" component over the AppBar.
This component would

compute the scroll based on this component and not from window.scrollY.
handle the positionning. I think that the AppBar should be static as a default.

A good candidate for this role is the AppCanvas.
Hence, I think that it should be the AppCanvas how should have the logic for the waterfall logic not the AppBar.
We would have an API that looks like:

<AppCanvas
  mode="waterfall"
  waterfallMinHeight={40}
  waterfallMaxHeight={400}
>
  <AppBar
    waterfallChildren={(height) => {
     return <MyComponent />; 
   }}
   />
  <div>
    Contente text.
  </div>
</AppCanvas>

oliviertassinari · 2015-12-12T18:21:13Z

src/app-bar.jsx

+     * the component) properties. Using onHeightChange, animation effects can be achieved
+     * by altering style properties of specific DOM elements.
+     */
+    waterfall: React.PropTypes.shape({


I think that it would be better to have a flat list of properties instead of an object.
It would be simpler to document and to implement an efficient shouldComponentUpdate.

I can do this, but please let me know if it's the only problem left. I don't want to keep changing things and then get anyway a reject because of another bigger things.

newoga · 2015-12-12T18:31:08Z

@oliviertassinari , I agree with looking into the approach of having a parent component manage scroll state

igorbt · 2015-12-13T07:18:50Z

@oliviertassinari, reagarding the independent example, unfortunately it's not possible with waterfall current implementation to put an in place example because it is relying on position fixed. But I didn't use there docs AppBar, that's a separate AppBar that appears on top of it. It's kind of similar approach with the LeftNav component demo.
That's a good idea of combining AppCanvas and AppBar to work together. Then it would be possible to implement all the requirements from the spec and also to make an in place example/demo.
Like I said before, there's room to do it better and implement all the specs. But I'm afraid I don't have the time to do it now, I already did some overtime on this PR. So please decide if you want to have this intermediate solution from this PR merged and then do the whole thing in a next iteration or rather close it and accept only when the whole thing is ready. I'll be OK with both decisions.

newoga · 2015-12-14T04:15:57Z

@igorbt I think think you started something really good here and I see this as an important step to getting the docs closer to using standard material-ui framework. I certainly understand time constraints though.

@oliviertassinari @subjectix I'm willing to help on this pull request to implement some of the remaining fixes, or to help out after if we decide to merge. I can also work off of @igorbt's branch.

alitaheri · 2015-12-14T06:01:27Z

@newoga I definitely like this feature. But @oliviertassinari is right, this feature needs to go to the right place. It would be great if you could help on this, me and @oliviertassinari are busy with the docs, I would have helped otherwise since @igorbt seems to be very busy to continue working on this. Just the rebase was a work of magic. still wondering how he pulled it off 😨 I think if @igorbt would give you access to his branch so you could continue there, it would be ideal. 😁

igorbt · 2015-12-14T08:08:05Z

@subjectix , it's no problem at all to give @newoga or whoever wants to work on this the access to the branch. I'm not sure how to do this but I'll find out if needed.

alitaheri · 2015-12-14T08:10:51Z

@igorbt Go to your fork -> settings -> collaborators -> add @newoga there.

Or he could just open pull requests on your fork. works both ways. The first though, is easier on both of you. no need to merge all the time.

igorbt · 2015-12-14T09:28:42Z

@subjectix I just added @newoga as a collaborator. So it would be great if he can give us a helping hand here.

newoga · 2015-12-14T16:37:57Z

@igorbt Thanks! I got an email confirmation earlier. I'll review the notes/feedback on this pull request and the branch in more detail later today and keep you in the loop.

Thanks everyone!

alitaheri · 2015-12-14T16:41:52Z

@newoga Thanks a lot for following up on it. This feature will make a huge number of people real happy, me included 😁 😁

newoga · 2015-12-16T07:01:13Z

@igorbt and all, I haven't forgotten about this. I decided to take some time this evening to review the branch and work with plain old css and javascript to make sure I understand exactly how feature works. Based on everyone's feedback, I'm also exploring a few different topics such as:

Not relying on style position: fixed so that the AppBar can be used not only on window but on inner divs (like in examples)
Making this approach more extensible so that we can implement other parts of the spec that deal with scrolling, for example by using <AppCanvas />.
Trying to reduce and/or eliminate dependencies on ReactDOM and window object by externalizing scroll and window state

I feel like I've made some progress and I'm definitely still planning on getting some delivered in the near term, thanks for your patience! 😄

alitaheri · 2015-12-16T07:47:00Z

@newoga There's no hurry. this is a huge feature, take as much time as necessary. And thanks a whole bunch for working on this.

Sure, not relay, but we should support it in my opinion.
Yeah I guess that's a better approach 👍 👍
Yes definitely.

newoga · 2016-01-23T02:02:59Z

@igorbt I decided to combine my efforts with this PR with making the AppBar more composable in general. Your work was not in vain! It was super helpful in terms of implementing the scrolling logic. I'd love you feedback when you get a chance:

http://newoga.github.io/material-ui-scrolling-techniques/
https://github.com/newoga/material-ui-scrolling-techniques

Probably one of the biggest remaining pieces is making the children also accept a function that gets passed the scroll position or some value to assist with user animations.

igorbt · 2016-01-23T08:56:53Z

@newoga demo looks awsome! I will have a closer look to the code.

igorbt · 2016-01-23T14:00:49Z

@newoga, I really like the composability and usage described in README.
The code looks clever, but I, have to say it, it was not so easy to follow. Maybe extracting the scrolling behavior in ScrollingTechniques out of AppBar added to the overall complexity, because you had to wrap each child in a Block component and pass back and forth some props to make them work together. I'm not sure if it's really worth it. I understand that's a challenging task to implement this and I saw you structured your components like in Material Design specs.
I'm wondering if you really need wrapping with Block components. Maybe just using refs to get to the DOM elements of the children is enough.

newoga · 2016-01-23T18:25:29Z

Thanks @igorbt!

Maybe extracting the scrolling behavior in ScrollingTechniques out of AppBar added to the overall complexity

This is true, to implement all the various scrolling behaviors of the spec, I realized we have to coordinate the modification of styling for multiple elements, sometimes but not always at the same scroll position.

because you had to wrap each child in a Block component and pass back and forth some props to make them work together. I'm not sure if it's really worth it.

The easiest way to extract AppBar's dependency on window and ReactDOM was to have some parent component handle it (window scrolling and style modification). That being said I agree! What I was going to try to simplify this was to remove the block component entirely (which would involve switching AppBar to a class so that it could assign refs to local div elements).

Thanks for the feedback @igorbt! I was going to try finish implementing the rest of the scrolling behavior before simplifying the code to make sure there's not a design flaw anywhere. I agree that the implementation is far from perfect, but I'd be curious what your thoughts are in terms of how the external API should work for modifying scrolling behavior.

igorbt · 2016-01-23T20:37:43Z

I'd be curious what your thoughts are in terms of how the external API should work for modifying scrolling behavior.

If you mean how API should be to enable user defined animations, then I would say it should be the most flexible way. I imagine that ScrollingBehavior component would accept one or more callback props that would be called when relevant scrolling events happen (like FlexibleSpace height changed, ToolBar position changed, etc). Then user can do whatever animations he likes either re-rendering by setting some variables in state, either by directly modifying the styles of some DOM elements that he has in some refs.

leesei · 2016-02-04T14:58:49Z

A.W.E.S.O.M.E feature and discussion.
I love the openness of the Material-UI core team.

And of course @igorbt's contribution in the first place.

newoga · 2016-02-27T00:46:08Z

Let's close this for now. We're still very much invested in implementing this, but I think cleaning up our list of PRs will make us more productive.

The composable AppBar is definitely planned for 0.15.0. When we have that, we can explore the waterfall/scrolling techniques in the context of that new component. I think we're getting there but another group review will be helpful. It will be easier to have then when we have a little less on our plate and we get some of other bigger pieces on the ROADMAP finished.

scharf · 2016-07-24T17:13:43Z

What is the status of the waterfall/scrolling?

atar-axis · 2019-07-10T00:10:08Z

Any news here?

oliviertassinari added the GH Review: review-needed label Oct 26, 2015

oliviertassinari reviewed Nov 10, 2015
View reviewed changes

igorbt force-pushed the waterfall-appbar branch from df16ebf to ef44b75 Compare December 10, 2015 21:32

implemented waterfall

42068dc

changed implementation for an optimized performance

igorbt force-pushed the waterfall-appbar branch from ef44b75 to 42068dc Compare December 10, 2015 22:07

alitaheri reviewed Dec 11, 2015
View reviewed changes

alitaheri mentioned this pull request Dec 11, 2015

Hero as a component? #1585

Closed

oliviertassinari reviewed Dec 11, 2015
View reviewed changes

oliviertassinari reviewed Dec 12, 2015
View reviewed changes

mbrookes mentioned this pull request Jan 29, 2016

[Docs] Add an example on how to use Tabs inside an AppBar #3005

Closed

oliviertassinari mentioned this pull request Jan 30, 2016

[Feature] Scrollable tabs #2861

Closed

c0b41 mentioned this pull request Feb 18, 2016

[AppBar] Scrolls off the page #1792

Closed

mbrookes added this to the 0.15.0 Release milestone Feb 23, 2016

newoga closed this Feb 27, 2016

alitaheri removed the PR: Needs Review label Feb 29, 2016

mbrookes mentioned this pull request May 8, 2016

[Docs] Update the Roadmap #4205

Closed

zannager added the component: app bar This is the name of the generic UI component, not the React module! label Mar 14, 2023

[AppBar] waterfall behaviour #1321

[AppBar] waterfall behaviour #1321

Conversation

igorbt commented Aug 2, 2015

igorbt commented Oct 29, 2015

c0b41 commented Oct 29, 2015

shaurya947 commented Nov 10, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pandaiolo commented Nov 24, 2015

shaurya947 commented Nov 24, 2015

igorbt commented Nov 24, 2015

alitaheri commented Dec 10, 2015

pandaiolo commented Dec 10, 2015

alitaheri commented Dec 10, 2015

alitaheri commented Dec 10, 2015

pandaiolo commented Dec 10, 2015

alitaheri commented Dec 10, 2015

igorbt commented Dec 10, 2015

igorbt commented Dec 10, 2015

newoga commented Dec 11, 2015

c0b41 commented Dec 11, 2015

igorbt commented Dec 11, 2015

alitaheri commented Dec 11, 2015

alitaheri commented Dec 11, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari commented Dec 12, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

newoga commented Dec 12, 2015

igorbt commented Dec 13, 2015

newoga commented Dec 14, 2015

alitaheri commented Dec 14, 2015

igorbt commented Dec 14, 2015

alitaheri commented Dec 14, 2015

igorbt commented Dec 14, 2015

newoga commented Dec 14, 2015

alitaheri commented Dec 14, 2015

newoga commented Dec 16, 2015

alitaheri commented Dec 16, 2015

newoga commented Jan 23, 2016

igorbt commented Jan 23, 2016

igorbt commented Jan 23, 2016

newoga commented Jan 23, 2016

igorbt commented Jan 23, 2016

leesei commented Feb 4, 2016

newoga commented Feb 27, 2016

scharf commented Jul 24, 2016

atar-axis commented Jul 10, 2019