feat(subheader): add subheader component #269
Conversation
| var next; // pointer to next target | ||
|
|
||
| // Convenience getter for the target element | ||
| var targetElementIndex = $container.data('$stickyTarget'), |
There was a problem hiding this comment.
It seems we're getting to the point where it would be better to just have one element.data('$sticky') object that contains .target, .orderedEls, etc
There was a problem hiding this comment.
Also if you do that, you could just have one stickyData var at the top, so you don't have to keep calling $container.data('$sticky'). Cleaner & easier.
There was a problem hiding this comment.
That depends on checkElements existing for each sticky container. I'd rather keep the memory footprint smaller by using the $container.data -- if you're opinionated on it, I'm happy to change it, just my initial thoughts.
|
This looks good overall! The main problem I see is that you're using css top to push elements. This is a problem because changing CSS top isn't GPU accelerated and will look really slow when doing it every frame on iOS & older android devices. Just use |
| incrementElement(); | ||
| } | ||
| //If we are going up, and our normal position would be rendered not sticky, un-sticky ourselves | ||
| else if(!scrollingDown && stickyActive && targetRect.top > 0) { |
There was a problem hiding this comment.
Coding convention: we always put the brace on the same line as the else if.
Just add a blank line if you want to space things out.
|
Regarding documentation: I would say don't bother doing that yet. We can just document material-subheader for now. |
|
|
||
|
|
||
| $el.attr({ | ||
| 'role' : $attr.role || Constant.ARIA.ROLE.HEADING |
There was a problem hiding this comment.
This is actually unnecessary because there is a heading element nested inside of it. Good intention, though!
|
👍 for making the heading sticky without having to modify the DOM. |
| $el = Util.wrap($el, 'div', 'sticky-container'), | ||
| $container = $el.controller('materialContent').$element; | ||
|
|
||
| if(!$container) { throw new Error('$materialSticky used outside of material-contant'); } |
There was a problem hiding this comment.
Need to check for presence of $el.controller('materialContent'), too.
rschmukler
force-pushed
the
wip-sub-header
branch
from
c7b643f to
bc706c4
Compare
| @@ -29,6 +29,28 @@ var Util = { | |||
| }, | |||
|
|
|||
| /** | |||
| * Returns a function, that, as long as it continues to be invoked, will not | |||
There was a problem hiding this comment.
Util.debounce already exists in master, once you rebase.
| // If we are scrollingDown, sticky, and are being pushed off screen by a different element, increment | ||
| if(scrollingDown && stickyActive && contentRect.bottom <= 0 && targetElementIndex < orderedElements.length - 1) { | ||
| targetElement().children(0).removeAttr('material-sticky-active'); | ||
| targetElement().css({height: null}); |
There was a problem hiding this comment.
Why do you use the object notation when you're only setting one property?
The object notation requires allocating a new object here, then looping through it internally.
It may seem inconsequential, but I'd rather save where we can if it's easy to save.
|
We've talked, and I know you can handle me being picky about little details. So I'm being really picky about little details...a little too much probably. The things that matter the most are |
rschmukler
force-pushed
the
wip-sub-header
branch
from
bc706c4 to
a58e137
Compare
ajoslin
force-pushed
the
wip-sub-header
branch
from
e594f28 to
9a9b8a5
Compare
rschmukler
force-pushed
the
wip-sub-header
branch
from
e866e69 to
ea217e9
Compare
|
Just a note, we should review this with the UX team -- Android has issues with scroll / rAF events firing after hardware accelerated scrolling redraws the screen. This makes some jank. @ajoslin |
rschmukler
force-pushed
the
wip-sub-header
branch
from
ea217e9 to
dfa5678
Compare
rschmukler
force-pushed
the
wip-sub-header
branch
from
dfa5678 to
ea217e9
Compare
ThomasBurleson
force-pushed
the
master
branch
from
56e270a to
fd7697d
Compare
ajoslin
force-pushed
the
wip-sub-header
branch
from
a9a47c0 to
41d562b
Compare
rschmukler
force-pushed
the
wip-sub-header
branch
from
41d562b to
c995ca6
Compare
This implements #216.
As per suggestion from @matsko, uses native position: sticky when possible, otherwise uses a javascript implementation using the new $materialSticky service.
A new directive material-sticky is also exposed, which uses the $materialSticky service.
This pull request also makes the material-subheader create a h2 for accessibility purposes as per @marcysutton's suggestion. Subsequently, it ups the h2 and h3 in material-list-item to h3 and h4 respectively.
Commit messages have been squashed
Would love suggestions from @ajoslin on ideas for testing $materialSticky service, or anyone else who has ideas.
Also does anyone have opinion on whether we want to document material-sticky directive / $materialSticky service for usage on the docs?