Skip to content
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

UI: Fixes healthy node listing resize on large portrait screens #4564

Merged
merged 3 commits into from
Aug 24, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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);
},
});

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice mixin. One suggestion I have is to put emphasis on resize in some manner to make it obvious that that is the interesting part for a consumer of this mixin. This could be as simple as moving resize to the first thing in the file so it's hard to overlook, or something more aggressive like this, which throws an error if a component/controller/whatever is mixing in the mixin without overriding the expected method.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

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(),

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neat. I need to look into ember-sinon-qunit. I'm just using plain sinon right now, but I love spies.

};
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);
});