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

Fix dashboard layouts when width or height is 0 #3766

Merged
merged 2 commits into from May 8, 2017

Conversation

Projects
None yet
4 participants
@edmundoa
Member

edmundoa commented May 3, 2017

Dashboards created from content packs always get a value for each widget dimensions, being 0 the default. This behaviour is different than what happens when a widget is created manually and is now taken into account.

Fixes #3765

@edmundoa edmundoa added this to the 2.3.0 milestone May 3, 2017

@@ -131,8 +134,8 @@ const ShowDashboardPage = React.createClass({
positions[widget.id] = {
col: (persistedDimensions.col === undefined ? defaultDimensions.col : persistedDimensions.col),
row: (persistedDimensions.row === undefined ? defaultDimensions.row : persistedDimensions.row),
height: (persistedDimensions.height === undefined ? defaultDimensions.height : persistedDimensions.height),
width: (persistedDimensions.width === undefined ? defaultDimensions.width : persistedDimensions.width),
height: (this._validDimension(persistedDimensions.height) ? persistedDimensions.height : defaultDimensions.height),

This comment has been minimized.

@dennisoelkers

dennisoelkers May 3, 2017

Member

Minor nitpick: why not just use persistedDimensions.height || defaultDimensions.height? It is shorter, saves a function call and makes the intention even clearer.

This comment has been minimized.

@edmundoa

edmundoa May 3, 2017

Member

For me this is by far more explicit, as I'm calling a function just to say this may have a different value than just undefined.

This comment has been minimized.

@dennisoelkers

dennisoelkers May 3, 2017

Member

Using || would do far more than just checking for undefined, it also checks for null and 0, so it is doing exactly the same.

This comment has been minimized.

@edmundoa

edmundoa May 3, 2017

Member

Yes, but it does not make the intention clearer. We use this shortcut all over the place when we can't be bother to look if it's undefined or null. Using || also checks for false and probably two or three other things. In this way I'm clearly stating that this value can be 0 even if it doesn't look like it.

height: (persistedDimensions.height === undefined ? defaultDimensions.height : persistedDimensions.height),
width: (persistedDimensions.width === undefined ? defaultDimensions.width : persistedDimensions.width),
height: (this._validDimension(persistedDimensions.height) ? persistedDimensions.height : defaultDimensions.height),
width: (this._validDimension(persistedDimensions.width) ? persistedDimensions.width : defaultDimensions.width),

This comment has been minimized.

@dennisoelkers

dennisoelkers May 3, 2017

Member

Same here.

@bernd

This comment has been minimized.

Member

bernd commented May 5, 2017

@dennisoelkers @edmundoa Can we move on with this? The discussion seems to be around a minor nitpick, not a real issue.

@@ -119,6 +119,9 @@ const ShowDashboardPage = React.createClass({
_dashboardIsEmpty(dashboard) {
return dashboard.widgets.length === 0;
},
_validDimension(dimension) {
return dimension !== null && dimension !== undefined && dimension !== 0;

This comment has been minimized.

@joschi

joschi May 6, 2017

Contributor

Shouldn't this be as follows (using Number.isInteger()):

return Number.isInteger(dimension) && dimension > 0;

This comment has been minimized.

@edmundoa

edmundoa May 8, 2017

Member

The problem is that Number.isInteger() is not implemented in IE. I'll check if the polyfill we use supports it.

This comment has been minimized.

@edmundoa

edmundoa May 8, 2017

Member

It works fine, so I changed the function 👍

edmundoa added some commits May 3, 2017

Fix dashboard layouts when width or height is 0
Dashboards created from content packs always get a value for each widget
dimensions, being 0 the default. This behaviour is different than what
happens when a widget is created manually and is now taken into
account.

Fixes #3765

@edmundoa edmundoa force-pushed the issue-3765 branch from 294d23d to f709e4d May 8, 2017

@joschi

joschi approved these changes May 8, 2017

@joschi joschi self-assigned this May 8, 2017

@joschi joschi merged commit e1779ba into master May 8, 2017

5 checks passed

ci-web-linter Jenkins build graylog-pr-linter-check 1608 has succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
graylog-project/pr Jenkins build graylog-project-pr-snapshot 65 has succeeded
Details
license/cla Contributor License Agreement is signed.
Details

@joschi joschi deleted the issue-3765 branch May 8, 2017

@joschi joschi removed the ready-for-review label May 8, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment