UX: include custom headers in --header-offset
#21059
Conversation
There was a problem hiding this comment.
We need to be extremely careful here - this is super performance-sensitive code. Happy to dig into the example tomorrow if nobody else beats me to it.
Also cc @pmusaraj since I think you have worked on this stuff quite a lot in the past.
|
@davidtaylorhq please do dig into this tomorrow, I won't have time to do so for the next few days. |
davidtaylorhq
force-pushed
the
custom-header-offset
branch
from
64e6e53
to
44882d3
Compare
Setting a style property causes the browser to recalculate styles, which can be quite time consuming. No need to do that if the value is unchanged. This should make this feature essentially 'free' on sites without a custom header. On sites with a custom header, it'll only cause style recalculations while --header-top is actually changing
…iscourse into custom-header-offset
|
ok finally got tests passing reliably again 😅 |
|
@davidtaylorhq I was just reminded this due to another issue, do you think this is ok to merge now... alternatively I could close this and start a dev todo if you think it's worth looking into other approaches |
|
Sorry for the delay @awesomerobot. I think we get get this into a mergeable state. My main remaining concern is the addition of |
|
@davidtaylorhq thanks! getting tests working at all was frustrating with this, so anything to get them more stable would be appreciated! |
When using vanilla JS, requestAnimationFrame makes sense. However, since (almost) all our rendering is handled by Ember, using `beforeRender` accomplishes the same thing, and makes things like tests much more reliable. e.g. Ember will ensure the runloop is drained before moving on to the next step of the test. The same cannot be done with the browser-native `requestAnimationFrame`.
`if(DEBUG)` means that the contents of this if statement will be optimized out of production builds.
These should no longer be required now that the calculation is fixed in test mode, and we're using Ember's runloop for scheduling
davidtaylorhq
force-pushed
the
custom-header-offset
branch
from
a4b1c16
to
e4d4b7a
Compare
davidtaylorhq
force-pushed
the
custom-header-offset
branch
from
b5f18b7
to
85be2de
Compare
|
@awesomerobot I've made some changes which I think should make this more robust - details in the commit messages. Let me know if you have any questions. If you're happy with the tweaks I've made, please feel free to merge |
|
This seems to work great, thank you so much for your help with this! 😍 there are at least a few theme components that this will solve minor issues for, as well as a number of customer themes. |
|
This pull request has been mentioned on Discourse Meta. There might be relevant details there: |
In a theme component I've been trying to compensate for a custom header in CSS by adding to
--header-offsetbut this doesn't cover whenheaderOffset()is used in JS: https://github.com/discourse/discourse-header-submenus/blob/b5661b04839a1e9b78eaf25541422f324b96eac6/desktop/desktop.scss#L18I documented the need for this earlier over here: https://meta.discourse.org/t/header-submenus-theme-component-pushes-bottom-of-sidebar-out-of-the-screen/255496/2?u=awesomerobot
So here I'm checking the distance of the
.d-header-wrapfrom the top of the page, and adjusting on scroll so this covers both sticky and non-sticky custom headers.Previously this was covered here in
minimumOffset(deprecated):discourse/app/assets/javascripts/discourse/app/lib/offset-calculator.js
Line 21 in 392bafc
headerOffset