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

Improving widget/grid stability and prop consistency #4160

Merged
merged 25 commits into from Sep 29, 2017
Merged

Conversation

@dennisoelkers
Copy link
Member

@dennisoelkers dennisoelkers commented Sep 18, 2017

This PR is cherry-picking a number of commits from the post-2.3-changes branch which aid in improving widget and grid stability and consistency.

Copy link
Member

@edmundoa edmundoa left a comment

Thank you for going through the commits and filtering the important ones!

This looks in general fine, although I added some questions/possible issues inline.

@@ -83,7 +87,7 @@ const MessageTablePaginator = React.createClass({
newPage = Number(page);
}

SearchStore.page = newPage;

This comment has been minimized.

@edmundoa

edmundoa Sep 21, 2017
Member

We should remove the import on SearchStore, it seems unused since this change.

This comment has been minimized.

@dennisoelkers

dennisoelkers Sep 22, 2017
Author Member

@@ -14,6 +14,11 @@ const FormUtils = {
return input.value;
}
},

// Emulates a minimal input event that can be used with FormUtils#getValueFromInput()
inputEvent(name, value) {

This comment has been minimized.

@edmundoa

edmundoa Sep 21, 2017
Member

This does not seems to be used at the moment, and I'm also not sure what is the reasoning for adding this code. If we need a fake event for some reason, I guess we should use at least an instance of CustomEvent.

if (nextProps.height !== this.props.height || nextProps.width !== this.props.width) {
this._resizeVisualization(nextProps.width, nextProps.height);
this.renderGraph(nextProps);

This comment has been minimized.

@edmundoa

edmundoa Sep 21, 2017
Member

Do we need to call renderGraph() here? It looks like it is updating the series names and resizing the visualization (which are the two things dependent on props as far as I can see). Both those properties are also being updated with custom methods, so I guess we can save a re-render on this case.

This comment has been minimized.

@dennisoelkers

dennisoelkers Sep 22, 2017
Author Member

What do you mean with "custom methods"? From my tests the graph is not properly resized without the following renderGraph() call.

This comment has been minimized.

@edmundoa

edmundoa Sep 22, 2017
Member

I mean that it looks like renderGraph(), _updateSeriesNames() and _resizeVisualization() are updating the same things, just at different times. If _renderGraph() is the one that is really needed to update the values, then I guess the other two methods are somehow redundant.

.attr('r', thresholdDotRadius)
.attr('rel', 'tooltip') // To make the tooltip helper pick it up
.attr('data-original-title', () => {
return `<div class="datapoint-info">${thresholdTooltip}</div>`;

This comment has been minimized.

@edmundoa

edmundoa Sep 21, 2017
Member

Will thresholdTooltip ever have some user defined value? Here we use d3 directly and are away from React's protection against code injection.

This comment has been minimized.

@dennisoelkers

dennisoelkers Sep 22, 2017
Author Member

Very good catch. Escaping this one.

getDefaultProps() {
return {
dataTableTitle: 'Top values',
limit: 50,

This comment has been minimized.

@edmundoa

edmundoa Sep 21, 2017
Member

This setting is being currently misused as far as I can see. We use it to set the table size, which I guess it's what we want. At the same time, we also use it to calculate the top values and as a cap for the number of slices in the pie chart.

This comment has been minimized.

@edmundoa

edmundoa Sep 21, 2017
Member

By the way, the changes removed all usages of the NUMBER_OF_TOP_VALUES variable, but the variable itself is still in the code. We should remove it if we don't need it any longer.

This comment has been minimized.

@dennisoelkers

dennisoelkers Sep 22, 2017
Author Member

Actually all three are required. The idea is to provide a Top-N widget, where n is the limit of the number of top items. I do not know if the .size(limit) statement is required, because the top group should also include limiting the table size, obviously.

Re: NUMBER_OF_TOP_VALUES:

This comment has been minimized.

@edmundoa

edmundoa Sep 22, 2017
Member

Okay, I understand that all three are required, but I think there are two different things we want to customize:

  1. Number of rows in the table (set in size(limit))
  2. Number of top values inside the visualization (which is used to highlight the n top values in the table, and is also used to select which fields will be in the pie chart). Those settings are handled by this.group.top(limit) and slicesCap(this.props.limit)

Currently with only one setting for everything, and being 50 the default, the table will only display 50 values, all of them will be marked as "top values", and the pie chart will display 51 values (the 50 "top values" plus the others categories, if we don't limit the amount of data somewhere else). I think this may be a particular case of the settings, but using the visualization as it was before should still be possible and makes sense in my opinion.

@dennisoelkers dennisoelkers force-pushed the widget-improvements branch from 0b8b505 to cf7d2b1 Sep 22, 2017
dennisoelkers and others added 24 commits Jul 18, 2017
Now it is possible to restrict widget resize, even if ReactGridContainer is
not locked.
This makes ReactGridContainer work even if not all position properties
are set.
- Use flexbox to make styling a bit simpler
- Move styling away from global stylesheet
Also namespace the renderlet calback in D3Utils.tooltipRenderlet() to
avoid conflicts with other callbacks.
Also rename a variable and remove static attributes from updating.
That way it doesn't look like another regular value.
@dennisoelkers dennisoelkers force-pushed the widget-improvements branch from 1588afd to df7adb7 Sep 22, 2017
@bernd
Copy link
Member

@bernd bernd commented Sep 26, 2017

@dennisoelkers @edmundoa Any progress on this?

@dennisoelkers
Copy link
Member Author

@dennisoelkers dennisoelkers commented Sep 26, 2017

No, not yet.

Copy link
Member

@edmundoa edmundoa left a comment

LGTM 👍

@edmundoa edmundoa merged commit ac4c9b0 into master Sep 29, 2017
5 checks passed
5 checks passed
@garybot2
ci-web-linter Jenkins build graylog-pr-linter-check 1950 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
@garybot2
graylog-project/pr Jenkins build graylog-project-pr-snapshot 510 has succeeded
Details
license/cla Contributor License Agreement is signed.
Details
@edmundoa edmundoa deleted the widget-improvements branch Sep 29, 2017
edmundoa added a commit that referenced this pull request Sep 29, 2017
* Fix linting issues in ReactGridContainer

* Add prop to set row height in ReactGridContainer

* Add extra flag to resize in ReactGridContainer

Now it is possible to restrict widget resize, even if ReactGridContainer is
not locked.

* Set default positioning values

This makes ReactGridContainer work even if not all position properties
are set.

* Improve numeric visualization styling

- Use flexbox to make styling a bit simpler
- Move styling away from global stylesheet

* Defining propTypes, passing through disableSurroundingSearch prop.

* Passing page size and current page.

* Allowing to pass sort order, title & limit to quick values vis.

* Using count dimension to fix limiting in QuickValuesVisualization.

* Using actual limit parameter instead of (removed) constant.

* Making axis labels configurable for histogram.

* Using updated computation time range when normalizing data.

* Allowing to pass custom title to stacked chart series.

* Allow disabling circles on stacked graphs.

* Use new props when formatting data for stacked graphs.

* Check if data points are not empty before deducing custom tick values.

* Using next props to rerender graph upon props update.

* Add an optional threshold line in the GraphVisualization component

Also namespace the renderlet calback in D3Utils.tooltipRenderlet() to
avoid conflicts with other callbacks.

* Fix threshold updating in GraphVisualization

* Update threshold dot title on update in GraphVisualization

Also rename a variable and remove static attributes from updating.

* Use a dashed line for the threshold

That way it doesn't look like another regular value.

* Removing unused import.

* Removing unused constant.

* Escaping threshold tooltip.

* Separating pie chart slice and data table row limits.

(cherry picked from commit ac4c9b0)
@edmundoa
Copy link
Member

@edmundoa edmundoa commented Sep 29, 2017

Cherry-picked in 2.4 branch in 6db7ae3

bernd added a commit that referenced this pull request Oct 5, 2017
Fixes an issue when no "pageSize" prop is provided. This broke in #4160.
dennisoelkers added a commit that referenced this pull request Oct 6, 2017
)

Fixes an issue when no "pageSize" prop is provided. This broke in #4160.
dennisoelkers added a commit that referenced this pull request Oct 6, 2017
)

Fixes an issue when no "pageSize" prop is provided. This broke in #4160.

(cherry picked from commit 5a22fc2)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants