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

[DT] Fix Issue #1280: Incorrect width when using percentage #1303

Open
wants to merge 7 commits into
base: dev-master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/datatable/HISTORY.md
Expand Up @@ -4,7 +4,8 @@ DataTable Change History
@VERSION@
------

* No changes.
* Correctly adds the width to the table view based on the source of the width
change. [Issue #1280] [Pull Request #1303]

3.13.0
------
Expand Down
10 changes: 6 additions & 4 deletions src/datatable/js/base.js
Expand Up @@ -342,7 +342,7 @@ Y.DataTable.Base = Y.Base.create('datatable', Y.Widget, [Y.DataTable.Core], {
@protected
**/
_defRenderViewFn: function (e) {
e.view.render();
e.view.render({ src: e.src });
},

/**
Expand Down Expand Up @@ -418,8 +418,7 @@ Y.DataTable.Base = Y.Base.create('datatable', Y.Widget, [Y.DataTable.Core], {
**/
_relayCoreAttrChange: function (e) {
var attr = (e.attrName === 'data') ? 'modelList' : e.attrName;

this.view.set(attr, e.newVal);
this.view.set(attr, e.newVal, { src: 'widget' });
},

/**
Expand Down Expand Up @@ -517,7 +516,10 @@ Y.DataTable.Base = Y.Base.create('datatable', Y.Widget, [Y.DataTable.Core], {
**/
syncUI: function () {
if (this.view) {
this.fire('renderView', { view: this.view });
this.fire('renderView', {
view: this.view,
src: 'widget'
});
}
},

Expand Down
147 changes: 83 additions & 64 deletions src/datatable/js/table.js
Expand Up @@ -185,6 +185,57 @@ Y.namespace('DataTable').TableView = Y.Base.create('table', Y.View, [], {
//-----------------------------------------------------------------------//
// Protected and private methods
//-----------------------------------------------------------------------//

/**
Constructor logic.

@method intializer
@param {Object} config Configuration object passed to the constructor
@protected
@since 3.6.0
**/
initializer: function (config) {
this.host = config.host;

this._initEvents();

this._extractDisplayColumns();

this.after('columnsChange', this._extractDisplayColumns, this);
},

/**
Cleans up state, destroys child views, etc.

@method destructor
@protected
**/
destructor: function () {
if (this.head && this.head.destroy) {
this.head.destroy();
}
delete this.head;

if (this.foot && this.foot.destroy) {
this.foot.destroy();
}
delete this.foot;

if (this.body && this.body.destroy) {
this.body.destroy();
}
delete this.body;

if (this._eventHandles) {
this._eventHandles.detach();
delete this._eventHandles;
}

if (this.tableNode) {
this.tableNode.remove().destroy(true);
}
},

/**
Updates the table's `summary` attribute.

Expand Down Expand Up @@ -218,7 +269,7 @@ Y.namespace('DataTable').TableView = Y.Base.create('table', Y.View, [], {
@since 3.6.0
**/
_afterWidthChange: function (e) {
this._uiSetWidth(e.newVal);
this._uiSetWidth(e.newVal, { src: e.src });
},

/**
Expand Down Expand Up @@ -322,7 +373,7 @@ Y.namespace('DataTable').TableView = Y.Base.create('table', Y.View, [], {

this._uiSetCaption(this.get('caption'));
this._uiSetSummary(this.get('summary'));
this._uiSetWidth(this.get('width'));
this._uiSetWidth(this.get('width'), { src: e.src });

if (this.head || e.headerView) {
if (!this.head) {
Expand Down Expand Up @@ -357,38 +408,6 @@ Y.namespace('DataTable').TableView = Y.Base.create('table', Y.View, [], {
this._bindUI();
},

/**
Cleans up state, destroys child views, etc.

@method destructor
@protected
**/
destructor: function () {
if (this.head && this.head.destroy) {
this.head.destroy();
}
delete this.head;

if (this.foot && this.foot.destroy) {
this.foot.destroy();
}
delete this.foot;

if (this.body && this.body.destroy) {
this.body.destroy();
}
delete this.body;

if (this._eventHandles) {
this._eventHandles.detach();
delete this._eventHandles;
}

if (this.tableNode) {
this.tableNode.remove().destroy(true);
}
},

/**
Processes the full column array, distilling the columns down to those that
correspond to cell data columns.
Expand Down Expand Up @@ -446,24 +465,6 @@ Y.namespace('DataTable').TableView = Y.Base.create('table', Y.View, [], {
});
},

/**
Constructor logic.

@method intializer
@param {Object} config Configuration object passed to the constructor
@protected
@since 3.6.0
**/
initializer: function (config) {
this.host = config.host;

this._initEvents();

this._extractDisplayColumns();

this.after('columnsChange', this._extractDisplayColumns, this);
},

/**
Relays attribute changes to the child Views.

Expand Down Expand Up @@ -497,12 +498,17 @@ Y.namespace('DataTable').TableView = Y.Base.create('table', Y.View, [], {
Creates the UI in the configured `container`.

@method render
@param {Object} options used to pass the renderer's source to the table
@return {TableView}
@chainable
**/
render: function () {
render: function (options) {
options || (options = {});

if (this.get('container')) {
this.fire('renderTable', {
src: options.src || null,

headerView : this.get('headerView'),
headerConfig: this.get('headerConfig'),

Expand Down Expand Up @@ -569,20 +575,33 @@ Y.namespace('DataTable').TableView = Y.Base.create('table', Y.View, [], {

@method _uiSetWidth
@param {Number|String} width The width to make the table
@param {Object} options Object used to pass the source of the width setter
to update the table's width correctly
@protected
@since 3.5.0
**/
_uiSetWidth: function (width) {
var table = this.tableNode;

// Table width needs to account for borders
table.setStyle('width', !width ? '' :
(this.get('container').get('offsetWidth') -
(parseInt(table.getComputedStyle('borderLeftWidth'), 10)||0) -
(parseInt(table.getComputedStyle('borderLeftWidth'), 10)||0)) +
'px');

table.setStyle('width', width);
_uiSetWidth: function (width, options) {
var table = this.tableNode,
isPercentage = !!(width && width.toString().charAt(width.length - 1) === '%');


// need to determine if the table view is inside a table widget
// if inside a widget, the table gets set to 100% width (contained by the table)
// if not inside a widget, apply width directly to table view
if (options && options.src === 'widget') {
Copy link
Contributor

Choose a reason for hiding this comment

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

This fork is to support the use of the table view standalone?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's correct. Using the existing tests with table view standalone, trying to get the fix to work for widget view required a fork and a passing of source.

if (isPercentage) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't a 100% width rule be applied via css and overwritten in js only if necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A 100% width doesn't allow for a "natural" width

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point.

table.setStyle('width', '100%');
} else {
// Table width needs to account for borders
table.setStyle('width', !width ? '' :
(this.get('container').get('offsetWidth') -
(parseInt(table.getComputedStyle('borderLeftWidth'), 10)||0) -
(parseInt(table.getComputedStyle('borderRightWidth'), 10)||0)
) + 'px');
}
} else {
table.setStyle('width', width);
}
},

/**
Expand Down
91 changes: 91 additions & 0 deletions src/datatable/tests/unit/assets/datatable-base-tests.js
Expand Up @@ -341,6 +341,97 @@ suite.add(new Y.Test.Case({
}
}));

suite.add(new Y.Test.Case({
name: 'render table with different widths',

'render table with percentage': function () {
var div = Y.Node.create('<div/>'),
dt = new Y.DataTable({
columns: [ 'a', 'b' ],
data: [
{ a: 1, b: 'foo' }
],
width: '50%'
});

div.setStyles({
border: '1px solid blue',
width: '400px'
});

Y.one('body').append(div);

dt.render(div);

var bb = dt.get('boundingBox');

Y.Assert.areSame(200, bb.get('offsetWidth'));
Y.Assert.areSame(200, bb.one('table').get('offsetWidth'));

dt.destroy(true);
div.remove(true);
},

'render table with pixel width': function () {
var div = Y.Node.create('<div/>'),
dt = new Y.DataTable({
columns: [ 'a', 'b' ],
data: [
{ a: 1, b: 'foo' }
],
width: '500px'
});

div.setStyles({
border: '1px solid blue',
width: '400px'
});

Y.one('body').append(div);

dt.render(div);

var bb = dt.get('boundingBox');

Y.Assert.areSame(500, bb.get('offsetWidth'));
Y.Assert.areSame(500, bb.one('table').get('offsetWidth'));

dt.destroy(true);
div.remove(true);
},

'render table with no width': function () {
var div = Y.Node.create('<div/>'),
dt = new Y.DataTable({
columns: [ 'a', 'b' ],
data: [
{ a: 1, b: 'foo' }
]
});

div.setStyles({
border: '1px solid blue',
width: '400px'
});

Y.one('body').append(div);

dt.render(div);

var bb = dt.get('boundingBox');

Y.Assert.areSame(400, bb.get('offsetWidth'));

// less than 100 because natural width is variable based on the font size
Y.Assert.isTrue(bb.one('table').get('offsetWidth') < 100);

dt.destroy(true);
div.remove(true);
}


}));

/*
suite.add(new Y.Test.Case({
name: "destroy",
Expand Down
Expand Up @@ -446,4 +446,4 @@ suite.add(new Y.Test.Case({
Y.Test.Runner.add(suite);


}, '@VERSION@' ,{requires:['datatable', 'datatable-paginator', 'node-event-simulate', 'test']});
}, '@VERSION@' ,{requires:['selector-css3', 'datatable', 'datatable-paginator', 'node-event-simulate', 'test']});