Skip to content

Commit

Permalink
UI: Fixes healthy node listing resize on large portrait screens (#4564)
Browse files Browse the repository at this point in the history
1. Split the resizing functionality of into a separate mixin to be
shared across components
2. Add basic integration tests to prove that everything is getting
called through out the lifetime of the app. I decided against unit
testing as there isn't really any isolated logic to be tested, more
checking that things are being called in the correct order etc i.e. the
integration is correct.

Adds assertion to with-resizing so its obvious to override `resize`
  • Loading branch information
johncowen authored Aug 24, 2018
1 parent a00a2dc commit 4ebd70e
Show file tree
Hide file tree
Showing 6 changed files with 103 additions and 32 deletions.
28 changes: 27 additions & 1 deletion ui-v2/app/components/list-collection.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,32 @@
import { computed, get } from '@ember/object';
import Component from 'ember-collection/components/ember-collection';
import style from 'ember-computed-style';
import WithResizing from 'consul-ui/mixins/with-resizing';
import qsaFactory from 'consul-ui/utils/qsa-factory';
const $$ = qsaFactory();

export default Component.extend({
export default Component.extend(WithResizing, {
tagName: 'div',
attributeBindings: ['style'],
height: 500,
style: style('getStyle'),
classNames: ['list-collection'],
getStyle: computed('height', function() {
return {
height: get(this, 'height'),
};
}),
resize: function(e) {
const $self = this.element;
const $appContent = [...$$('main > div')][0];
if ($appContent) {
const rect = $self.getBoundingClientRect();
const $footer = [...$$('footer[role="contentinfo"]')][0];
const space = rect.top + $footer.clientHeight;
const height = e.detail.height - space;
this.set('height', Math.max(0, height));
this.updateItems();
this.updateScrollPosition();
}
},
});
37 changes: 7 additions & 30 deletions ui-v2/app/components/tabular-collection.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import needsRevalidate from 'ember-collection/utils/needs-revalidate';
import identity from 'ember-collection/utils/identity';
import Grid from 'ember-collection/layouts/grid';
import SlotsMixin from 'ember-block-slots';
import WithResizing from 'consul-ui/mixins/with-resizing';
import style from 'ember-computed-style';
import qsaFactory from 'consul-ui/utils/qsa-factory';

Expand All @@ -21,14 +22,6 @@ import { computed, get, set } from '@ember/object';

// ember doesn't like you using `$` hence `$$`
const $$ = qsaFactory();
// basic pseudo CustomEvent interface
// TODO: use actual custom events once I've reminded
// myself re: support/polyfills
const createSizeEvent = function(detail) {
return {
detail: { width: window.innerWidth, height: window.innerHeight },
};
};
// need to copy Cell in wholesale as there is no way to import it
// there is no change made to `Cell` here, its only here as its
// private in `ember-collection`
Expand Down Expand Up @@ -136,7 +129,7 @@ const change = function(e) {
}
}
};
export default Component.extend(SlotsMixin, {
export default Component.extend(SlotsMixin, WithResizing, {
tagName: 'table',
attributeBindings: ['style'],
width: 1150,
Expand All @@ -149,47 +142,31 @@ export default Component.extend(SlotsMixin, {
this.confirming = [];
// TODO: The row height should auto calculate properly from the CSS
this['cell-layout'] = new ZIndexedGrid(get(this, 'width'), 50);
this.handler = () => {
this.resize(createSizeEvent());
};
},
getStyle: computed('height', function() {
return {
height: get(this, 'height'),
};
}),
willRender: function() {
this._super(...arguments);
this.set('hasActions', this._isRegistered('actions'));
},
didInsertElement: function() {
this._super(...arguments);
// TODO: Consider moving all DOM lookups here
// this seems to be the earliest place I can get them
window.addEventListener('resize', this.handler);
this.didAppear();
},
willDestroyElement: function() {
window.removeEventListener('resize', this.handler);
},
didAppear: function() {
this.handler();
},
resize: function(e) {
const $tbody = [...$$('tbody', this.element)][0];
const $appContent = [...$$('main > div')][0];
if ($appContent) {
const rect = $tbody.getBoundingClientRect();
const $footer = [...$$('footer[role="contentinfo"]')][0];
const space = rect.top + $footer.clientHeight;
const height = new Number(e.detail.height - space);
const height = e.detail.height - space;
this.set('height', Math.max(0, height));
// TODO: The row height should auto calculate properly from the CSS
this['cell-layout'] = new ZIndexedGrid($appContent.clientWidth, 50);
this.updateItems();
this.updateScrollPosition();
}
},
willRender: function() {
this._super(...arguments);
this.set('hasActions', this._isRegistered('actions'));
},
// `ember-collection` bug workaround
// https://github.com/emberjs/ember-collection/issues/138
_needsRevalidate: function() {
Expand Down
30 changes: 30 additions & 0 deletions ui-v2/app/mixins/with-resizing.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
import Mixin from '@ember/object/mixin';
import { get } from '@ember/object';
import { assert } from '@ember/debug';
export default Mixin.create({
resize: function(e) {
assert('with-resizing.resize needs to be overridden', false);
},
win: window,
init: function() {
this._super(...arguments);
this.handler = e => {
const win = e.target;
this.resize({
detail: { width: win.innerWidth, height: win.innerHeight },
});
};
},
didInsertElement: function() {
this._super(...arguments);
get(this, 'win').addEventListener('resize', this.handler);
this.didAppear();
},
didAppear: function() {
this.handler({ target: get(this, 'win') });
},
willDestroyElement: function() {
get(this, 'win').removeEventListener('resize', this.handler);
this._super(...arguments);
},
});
2 changes: 1 addition & 1 deletion ui-v2/app/styles/components/list-collection.scss
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
.healthy > div {
width: calc(100% + 23px);
}
%card-grid {
.unhealthy > div {
margin-bottom: 20px;
}
%card-grid > ul,
Expand Down
26 changes: 26 additions & 0 deletions ui-v2/tests/integration/mixins/with-resizing-test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
import { module } from 'qunit';
import test from 'ember-sinon-qunit/test-support/test';
import { setupTest } from 'ember-qunit';
import EmberObject from '@ember/object';
import Mixin from 'consul-ui/mixins/with-resizing';
module('Integration | Mixin | with-resizing', function(hooks) {
setupTest(hooks);
test('window.addEventListener, resize and window.removeEventListener are called once each through the entire lifecycle', function(assert) {
const win = {
innerWidth: 0,
innerHeight: 0,
addEventListener: this.stub(),
removeEventListener: this.stub(),
};
const subject = EmberObject.extend(Mixin, {
win: win,
}).create();
const resize = this.stub(subject, 'resize');
subject.didInsertElement();
subject.willDestroyElement();
assert.ok(win.addEventListener.calledOnce);
assert.ok(resize.calledOnce);
assert.ok(resize.calledWith({ detail: { width: 0, height: 0 } }));
assert.ok(win.removeEventListener.calledOnce);
});
});
12 changes: 12 additions & 0 deletions ui-v2/tests/unit/mixins/with-resizing-test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
import EmberObject from '@ember/object';
import WithResizingMixin from 'consul-ui/mixins/with-resizing';
import { module, test } from 'qunit';

module('Unit | Mixin | with resizing');

// Replace this with your real tests.
test('it works', function(assert) {
let WithResizingObject = EmberObject.extend(WithResizingMixin);
let subject = WithResizingObject.create();
assert.ok(subject);
});

0 comments on commit 4ebd70e

Please sign in to comment.