Sticky header#101
Conversation
…e only other animation, into a file animations.scss
…tion to check and see if anything sticky related should change
…p to be the same thing as it was before, so there is no need for all of this conditional stuff
…the transition a bit
| @@ -1,96 +1,99 @@ | |||
| <div class="header" id="header"> | |||
There was a problem hiding this comment.
oh god this diff i'm sorry lol
There was a problem hiding this comment.
So AFAICT the diff looks 🍌 s here but pretty much the only change is the addition of the ref and dynamic -sticky class to the top level element, and the stuck header placeholder at the bottom, eh?
There was a problem hiding this comment.
it's true! just classic tabbing + diff disaster
| } | ||
| }, | ||
| mounted() { | ||
| const checkIfHeaderShouldBeSticky = function() { |
There was a problem hiding this comment.
I tested this method pretty rigorously to make sure it doesn't make any unnecessary render calls. It only triggers an update when a) the header height changes (which is usually only twice) or b) the stickyness changes.
| }, | ||
| mounted() { | ||
| const checkIfHeaderShouldBeSticky = function() { | ||
| this.headerHeight = (this.$refs.header) ? this.$refs.header.clientHeight : null; |
There was a problem hiding this comment.
this is just a read so I think it is safe to call over and over again lol
|
ok @stuartsan this is about ready to go. Let me know what you think! |
| } | ||
| } | ||
| }, | ||
| mounted() { |
There was a problem hiding this comment.
I feel like we probably don't want to run this repeatedly in an interval....what about a debounced scroll handler, so it is responsive to the user's interaction but also not too inefficient/blocking because it won't execute on every single scroll event? Something like:
(I feel like it is probably ok to assume the header height is a fixed thing too rather than read it each time)
There was a problem hiding this comment.
(Might have to also add an event listener for touchmove, e.g. http://stackoverflow.com/a/9787064)
There was a problem hiding this comment.
ok yup! debounce is good. I've traditionally done something like that, but saw a vue implementation that used setInterval and thought it sorta made sense since sticky elements aren't expected to be super responsive.
But yeah you are probably right in retrospect that was maybe a ¯\_(ツ)_/¯ idea lol
There was a problem hiding this comment.
Haha, everything is a ¯\_(ツ)_/¯ idea 😁 Ok, cool.
| text-align: center; | ||
| } | ||
|
|
||
| .header.-sticky { |
There was a problem hiding this comment.
One issue when hiding the branding and ending up with a new header height is that if you click on the filter events button, you end up with:
Where the height for the filters thing is set incorrectly
There was a problem hiding this comment.
dang! i even tested this menu but must have not done the right combo of things. I will fix!
|
Overall this looks really nice and I dig the animation! I had just a couple comments, one bug and one idea for an improvement. |
…ry time which is maybe bad and also the variables seem to be binding to window which is DEFINITELY bad
| store.dispatch('setFilters',{zipcode: newZipcode }); | ||
| } | ||
| }, | ||
| headerHeight: function() { |
There was a problem hiding this comment.
I'm not in love with this, but the header changes height several times as different assets come in, and setting it only once during mount isn't gonna be good enough. I'm open to better ideas though!
…ce the header was made sticky
|
@stuartsan ok round 2 for your leisurely perusal. One controversial line in |
|
Ah, sorry @Chrissy ! I forgot to take a look at this, will look soon. |
| headerHeight: function() { | ||
| return (this.$refs.header) ? this.$refs.header.clientHeight : null; | ||
| }, | ||
| checkIfHeaderShouldBeStuck() { |
There was a problem hiding this comment.
Naming nit / suggestion, since this is not just checking but also setting the header's stickiness or non-stickiness, maybe something like setStickyHeader vs checkIfHeaderShouldBeStuck?
|
@Chrissy , sorry for my delayed response here. I added a couple thoughts, overall it looks great. There are some conflicts in Header.html that I'm not sure how to resolve correctly -- if you wouldn't mind resolving those, let's merge this thang! |
|
@stuartsan nice! I will fix the conflicts and address the comment this evening. |
|
ok everything is conflict-free and I made that one change! @stuartsan |
2 participants
It's a sticky header!