-
Notifications
You must be signed in to change notification settings - Fork 555
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix header height calculation for large titles #1859
Conversation
setHeaderHeightStyle(headerElement) { | ||
if (headerElement) { | ||
const height = headerElement.clientHeight || 0; | ||
document.documentElement.style.setProperty('--header-height', `${height}px`); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would appreciate some ideas on how to test this. I would like to somehow mock the getting of the ref but I can't seem to do this properly with jest.spyOn(..)
.
References:
How to mock ref properties (GH)
How to mock ref properties (SO)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I put a suggestion here: fix/header-height...header-height-test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Unfortunately it suffers from the same problem that the current master has - because it's executed from componentWillMount
, the height is set once but then never recalculated if the height changes (switching tabs, etc).
However, I've managed to fix this using createNodeMock
: facebook/react#7740 (comment)
Added a new test in e8c0dbf
['href', 'protocol', 'host', 'hostname', 'origin', 'port', 'pathname', 'search', 'hash'].forEach( | ||
prop => { | ||
let value = parser[prop]; | ||
if (prop === 'origin' && options.noOrigin) { | ||
value = null; | ||
} | ||
Object.defineProperty(window.location, prop, { | ||
value, | ||
writable: true | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just formatting, nothing changed here.
* Moved header height calculation to a root CSS property * Added basic tests for Header component * Added test for setting document element
* Moved header height calculation to a root CSS property * Added basic tests for Header component * Added test for setting document element
Changes
This PR changes the way the header height is calculated. Previously it used to get the height once on component mount, but this proved to be problematic when the title was updated after the component was rendered and a large title caused the header height to change.
Now, the height is calculated when the
Header
component is rendered, and the height is set as a CSS variable on the document root. This should also be more performant as the old state change would cause the component to re-render.The downside is that this is difficult to test. However, basic smoke tests for
Header
were missing, which have now been added.Preview
References
Raised internally through an ESD ticket.
Testing
Please describe how this can be tested by reviewers. Be specific about anything not tested and reasons why. If this library has unit and/or integration testing, tests should be added for new functionality and existing tests should complete without errors.
Checklist