Fix settings menu title cut off by scrollbar #599
Conversation
|
Verified that @JustinHoldstock has signed the CLA. Thanks for the pull request! |
src/lib/viewers/media/Settings.js
Outdated
| const { children } = menu; | ||
|
|
||
| // If we have enough children to require scrolling, take into account scroll bar width. | ||
| const scrollPadding = children.length > 7 ? 32 : 18; |
There was a problem hiding this comment.
Extract these magic numbers to constants
There was a problem hiding this comment.
Would looking at scrollHeight help?
The Element.scrollHeight read-only property is a measurement of the height of an element's content, including content not visible on the screen due to overflow.
https://developer.mozilla.org/en-US/docs/Web/API/Element/scrollHeight
src/lib/viewers/media/Settings.js
Outdated
| // height = n * $item-height + 2 * $padding (see Settings.scss) + 2 * border (see Settings.scss) | ||
| // where n is the number of displayed items in the menu | ||
| const sumHeight = [].reduce.call(menu.children, (sum, child) => sum + child.offsetHeight, 0); | ||
| const sumHeight = [].reduce.call(children, (sum, child) => sum + child.offsetHeight, 0); |
There was a problem hiding this comment.
I was taking a look at this before and was trying to figure out why it iterates over the children and sums up the offsetHeight vs taking the menu.offsetHeight. Seemed to work with the menu offsetHeight. Any ideas?
There was a problem hiding this comment.
I'm not too sure, but I can confirm that menu.offsetHeight is the way to go.
src/lib/viewers/media/Settings.js
Outdated
| const { children } = menu; | ||
|
|
||
| // If we have enough children to require scrolling, take into account scroll bar width. | ||
| const scrollPadding = children.length > 7 ? 32 : 18; |
There was a problem hiding this comment.
Would looking at scrollHeight help?
The Element.scrollHeight read-only property is a measurement of the height of an element's content, including content not visible on the screen due to overflow.
https://developer.mozilla.org/en-US/docs/Web/API/Element/scrollHeight
| it('should add extra padding to settingsEl based on menu contents that require scroll bar', () => { | ||
| const menuEl = document.createElement('div'); | ||
| // Greater than enough to add a scroll bar. | ||
| menuEl.appendChild(document.createElement('span')); |
There was a problem hiding this comment.
Add one element and set it's size with js?
There was a problem hiding this comment.
This will work especially since we're no longer counting child heights, and using offsetHeight
Fixes scroll bar cutting off menu