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 search results console warnings #2527

Merged
merged 3 commits into from Jul 28, 2016

Conversation

Projects
None yet
3 participants
@edmundoa
Member

edmundoa commented Jul 25, 2016

Expanding the message details from a message in the search result may lead to two console warnings:

  • Failed propType: Invalid prop 'value' of type 'number' supplied to 'MessageField', expected 'string'. Check the render method of 'MessageFields': This one is triggered when one of the field values of the message is not a string. The PR fixes it by converting it into a string.
  • Each child in an array or iterator should have a unique "key" prop. Check the render method of 'MessageFields': There were some arrays of React nodes that did not provide unique keys, so I added them.

edmundoa added some commits Jul 25, 2016

Pass string property to MessageField
The `MessageField` component expects an string value prop, so we convert
the value to string before passing it to the component.

@edmundoa edmundoa added this to the 2.1.0 milestone Jul 25, 2016

@dennisoelkers dennisoelkers self-assigned this Jul 25, 2016

@@ -19,7 +19,7 @@ const MessageFields = React.createClass({

_formatFields(fields, showDecoration) {
if (!showDecoration || !this.props.message.decoration_stats) {
return Object.keys(fields).sort().map(key => <MessageField {...this.props} fieldName={key} value={fields[key]} />);
return Object.keys(fields).sort().map(key => <MessageField key={key} {...this.props} fieldName={key} value={String(fields[key])} />);

This comment has been minimized.

@dennisoelkers

dennisoelkers Jul 25, 2016

Member

Do we really want to convert it into a string? Wouldn't it make more sense to relax the prop type for the component? It might make sense to still have the original type in the component, so we could display numbers/whatever differently.

This comment has been minimized.

@edmundoa

edmundoa Jul 25, 2016

Member

The only problem I see with relaxing the prop, is that some types may not render properly, like booleans. As at the moment we don't do anything special with the value, I don't see any problem with converting it to a string to be honest.

This comment has been minimized.

@dennisoelkers

dennisoelkers Jul 26, 2016

Member

We should still do the String conversion in the MessageField component, not externally.

This comment has been minimized.

@edmundoa

edmundoa Jul 27, 2016

Member

I don't know why we should do it in MessageField, I guess the best place would be in MessageFieldDescription, which is the component rendering the field and should be the one knowing how to render the field value.

This comment has been minimized.

@edmundoa

edmundoa Jul 27, 2016

Member

Looking at the code, MessageFieldDescription only uses the field value to add it to the search bar if that action is required. In the end possiblyHighlight() is converting and returning the value we want to display already, so I don't think we need to convert anything at this point. I will simply relax the prop as you suggested in the first place.

}

return <MessageField {...this.props} fieldName={key} value={fields[key]} disableFieldActions={true} />;
return <MessageField key={key} {...this.props} fieldName={key} value={String(fields[key])} disableFieldActions={true} />;

This comment has been minimized.

@dennisoelkers

dennisoelkers Jul 25, 2016

Member

See above

Accept any type in MessageField value prop
Delay the decision to convert the field value until we want to render
it.
@joschi

This comment has been minimized.

Contributor

joschi commented Jul 28, 2016

LGTM. 👍

@joschi joschi self-assigned this Jul 28, 2016

@joschi joschi merged commit 33d02b2 into master Jul 28, 2016

4 checks passed

ci-server-integration Jenkins build graylog2-server-integration-pr 1169 has succeeded
Details
ci-web-linter Jenkins build graylog-pr-linter-check 652 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

@joschi joschi deleted the fix-search-console-warnings branch Jul 28, 2016

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