Skip to content

Commit

Permalink
Fix settings menu title cut off by scrollbar (#599)
Browse files Browse the repository at this point in the history
COXP-4461
  • Loading branch information
JustinHoldstock authored Jan 24, 2018
1 parent a2397ce commit cdb627e
Show file tree
Hide file tree
Showing 2 changed files with 41 additions and 6 deletions.
16 changes: 10 additions & 6 deletions src/lib/viewers/media/Settings.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,9 @@ const MEDIA_SPEEDS = ['0.5', '1.0', '1.25', '1.5', '2.0'];
const MEDIA_QUALITY_SD = 'sd';
const MEDIA_QUALITY_HD = 'hd';
const MEDIA_QUALITY_AUTO = 'auto';
const SETTINGS_MENU_PADDING = 18;
const SETTINGS_MENU_PADDING_SCROLL = 32;
const SETTINGS_MENU_MAX_HEIGHT = 210;

const SETTINGS_TEMPLATE = `<div class="bp-media-settings">
<div class="bp-media-settings-menu-main bp-media-settings-menu" role="menu">
Expand Down Expand Up @@ -292,12 +295,13 @@ class Settings extends EventEmitter {
* @return {void}
*/
setMenuContainerDimensions(menu) {
// NOTE: need to explicitly set the dimensions in order to get css transitions. width=auto doesn't work with css transitions
this.settingsEl.style.width = `${menu.offsetWidth + 18}px`;
// 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);
this.settingsEl.style.height = `${sumHeight + 18}px`;
const paddedHeight = menu.offsetHeight + SETTINGS_MENU_PADDING;
this.settingsEl.style.height = `${paddedHeight}px`;

// If the menu grows tall enough to require scrolling, take into account scroll bar width.
const scrollPadding =
paddedHeight >= SETTINGS_MENU_MAX_HEIGHT ? SETTINGS_MENU_PADDING_SCROLL : SETTINGS_MENU_PADDING;
this.settingsEl.style.width = `${menu.offsetWidth + scrollPadding}px`;
}

/**
Expand Down
31 changes: 31 additions & 0 deletions src/lib/viewers/media/__tests__/Settings-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,37 @@ describe('lib/viewers/media/Settings', () => {
});
});

describe('setMenuContainerDimensions', () => {
it('should add padding to settingsEl based on menu contents and additional padding', () => {
const menuEl = document.createElement('div');
menuEl.appendChild(document.createElement('span'));
settings.setMenuContainerDimensions(menuEl);

expect(settings.settingsEl.style.width).to.equal('18px');
});

it('should add extra padding to settingsEl based on menu contents that require scroll bar', () => {
const menuEl = {
offsetWidth: 0,
offsetHeight: 500 // 210 is max height of settings menu
};
settings.setMenuContainerDimensions(menuEl);

expect(settings.settingsEl.style.width).to.equal('32px');
});

it('should grow the height of the settingsEl to that of the sub-menu, with padding', () => {
const menuEl = {
offsetWidth: 0,
offsetHeight: 18
};
settings.setMenuContainerDimensions(menuEl);

// Adds 18px to the offsetHeight of sum of child element's heights
expect(settings.settingsEl.style.height).to.equal('36px');
});
});

describe('destroy()', () => {
it('should remove event listeners on settings element and document', () => {
sandbox.stub(settings.settingsEl, 'removeEventListener');
Expand Down

0 comments on commit cdb627e

Please sign in to comment.