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

[Vis: Default editor] EUIficate order and size controls #35134

Merged
merged 14 commits into from
May 3, 2019
Merged
Show file tree
Hide file tree
Changes from 9 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
14 changes: 14 additions & 0 deletions src/legacy/ui/public/agg_types/__tests__/agg_params.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import { AggParams } from '../agg_params';
import { BaseParamType } from '../param_types/base';
import { FieldParamType } from '../param_types/field';
import { OptionedParamType } from '../param_types/optioned';
import { SelectParamType } from '../param_types/select';

describe('AggParams class', function () {

Expand Down Expand Up @@ -63,6 +64,19 @@ describe('AggParams class', function () {
expect(aggParams[0]).to.be.a(OptionedParamType);
});

it('Uses the SelectParamType class for params of type "select"', function () {
const params = [
{
name: 'order',
type: 'select'
}
];
const aggParams = new AggParams(params);

expect(aggParams).to.have.length(params.length);
expect(aggParams[0]).to.be.a(SelectParamType);
});

it('Always converts the params to a BaseParamType', function () {
const params = [
{
Expand Down
3 changes: 3 additions & 0 deletions src/legacy/ui/public/agg_types/agg_params.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,14 +20,17 @@
import { IndexedArray } from '../indexed_array';
import { FieldParamType } from './param_types/field';
import { OptionedParamType } from './param_types/optioned';
import { SelectParamType } from './param_types/select';
import { StringParamType } from './param_types/string';
import { JsonParamType } from './param_types/json';
import { BaseParamType } from './param_types/base';
import { createLegacyClass } from '../utils/legacy_class';

const paramTypeMap = {
field: FieldParamType,
// todo: remove select type and update optioned type after EUIfication of all components;
optioned: (OptionedParamType),
select: SelectParamType,
sulemanof marked this conversation as resolved.
Show resolved Hide resolved
string: (StringParamType),
json: (JsonParamType),
_default: (BaseParamType)
Expand Down
27 changes: 27 additions & 0 deletions src/legacy/ui/public/agg_types/buckets/_inline_comp_wrapper.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
/*
* Licensed to Elasticsearch B.V. under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch B.V. licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

sulemanof marked this conversation as resolved.
Show resolved Hide resolved
import React from 'react';

const wrapWithInlineComp = Component => props => (
<div className={`visEditorAggParam--inline visEditorAggParam--inline-${props.aggParam.name}`}>
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that if you go the route of renaming this to explicitly state half then you won't need to depend on the props.aggParam.name and the margin can be added to a sibling CSS selector like: .visEditorAggParam--half + .visEditorAggParam--half

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately the solution wouldn't work because of our current structure (both them are built through the existing interface and are wrapped into additional elements). They are not siblings and that was the main pain of positioning them both since the start.

order-and-size-siblings

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cchaos are you OK with current implementation ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I just have a concern around using the aggParam name.

Will props.aggParam.name eve have a space in it like "name here"? If so you'll end up with a className list like: visEditorAggParam--inline visEditorAggParam--inline-name here which causes .here to be a valid class which is not good.

I also still think you can change the class from --inline to --half so it's easily understood that the CSS will make that field 50% wide.

Copy link
Contributor Author

@sulemanof sulemanof Apr 30, 2019

Choose a reason for hiding this comment

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

Will props.aggParam.name eve have a space in it like "name here"? If so you'll end up with a className list like: visEditorAggParam--inline visEditorAggParam--inline-name here which causes .here to be a valid class which is not good.

It never can have a space in a name because we are using these names in a url string. These names should always have or camelCase style or _ between words, like intervalBase, ip_range, min_doc_count, extended_bounds and so on.

I also still think you can change the class from --inline to --half so it's easily understood that the CSS will make that field 50% wide.

This work has been already done in this commit

<Component {...props} />
</div>);

export { wrapWithInlineComp };
7 changes: 4 additions & 3 deletions src/legacy/ui/public/agg_types/buckets/significant_terms.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,10 @@
* under the License.
*/

import { i18n } from '@kbn/i18n';
import { SizeParamEditor } from '../controls/size';
import { BucketAggType } from './_bucket_agg_type';
import { createFilterTerms } from './create_filter/terms';
import orderAndSizeTemplate from '../controls/order_and_size.html';
import { i18n } from '@kbn/i18n';
import { isStringType, migrateIncludeExcludeFormat } from './migrate_include_exclude_format';

export const significantTermsBucketAgg = new BucketAggType({
Expand All @@ -47,7 +47,8 @@ export const significantTermsBucketAgg = new BucketAggType({
},
{
name: 'size',
editor: orderAndSizeTemplate,
editorComponent: SizeParamEditor,
default: '',
},
{
name: 'exclude',
Expand Down
29 changes: 16 additions & 13 deletions src/legacy/ui/public/agg_types/buckets/terms.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,9 @@ import { AggConfig } from '../../vis/agg_config';
import { Schemas } from '../../vis/editors/default/schemas';
import { createFilterTerms } from './create_filter/terms';
import orderAggTemplate from '../controls/order_agg.html';
import orderAndSizeTemplate from '../controls/order_and_size.html';
import { OrderParamEditor } from '../controls/order';
import { SizeParamEditor } from '../controls/size';
import { wrapWithInlineComp } from './_inline_comp_wrapper';
sulemanof marked this conversation as resolved.
Show resolved Hide resolved
import otherBucketTemplate from '../controls/other_bucket.html';
import { i18n } from '@kbn/i18n';

Expand Down Expand Up @@ -56,7 +58,7 @@ export const termsBucketAgg = new BucketAggType({
}),
makeLabel: function (agg) {
const params = agg.params;
return agg.getFieldDisplayName() + ': ' + params.order.display;
return agg.getFieldDisplayName() + ': ' + params.order.text;
},
getFormat: function (bucket) {
return {
Expand Down Expand Up @@ -117,10 +119,6 @@ export const termsBucketAgg = new BucketAggType({
type: 'field',
filterFieldTypes: ['number', 'boolean', 'date', 'ip', 'string']
},
{
name: 'size',
default: 5
},
{
name: 'orderAgg',
type: AggConfig,
Expand Down Expand Up @@ -203,7 +201,7 @@ export const termsBucketAgg = new BucketAggType({
}
},
write: function (agg, output, aggs) {
const dir = agg.params.order.val;
const dir = agg.params.order.value;
const order = output.params.order = {};

let orderAgg = agg.params.orderAgg || aggs.getResponseAggById(agg.params.orderBy);
Expand Down Expand Up @@ -240,25 +238,30 @@ export const termsBucketAgg = new BucketAggType({
},
{
name: 'order',
type: 'optioned',
type: 'select',
default: 'desc',
editor: orderAndSizeTemplate,
editorComponent: wrapWithInlineComp(OrderParamEditor),
options: [
{
display: i18n.translate('common.ui.aggTypes.buckets.terms.orderDescendingTitle', {
text: i18n.translate('common.ui.aggTypes.buckets.terms.orderDescendingTitle', {
defaultMessage: 'Descending',
}),
val: 'desc'
value: 'desc'
},
{
display: i18n.translate('common.ui.aggTypes.buckets.terms.orderAscendingTitle', {
text: i18n.translate('common.ui.aggTypes.buckets.terms.orderAscendingTitle', {
defaultMessage: 'Ascending',
}),
val: 'asc'
value: 'asc'
}
],
write: _.noop // prevent default write, it's handled by orderAgg
},
{
name: 'size',
editorComponent: wrapWithInlineComp(SizeParamEditor),
default: 5
},
{
name: 'orderBy',
write: _.noop // prevent default write, it's handled by orderAgg
Expand Down
62 changes: 62 additions & 0 deletions src/legacy/ui/public/agg_types/controls/order.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
/*
* Licensed to Elasticsearch B.V. under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch B.V. licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

import React, { useEffect } from 'react';
import { EuiFormRow, EuiSelect } from '@elastic/eui';
import { i18n } from '@kbn/i18n';
import { AggParamEditorProps } from 'ui/vis/editors/default';
import { SelectValueProp, SelectParamEditorProps } from '../param_types/select';

function OrderParamEditor({
aggParam,
value,
isInvalid,
setValue,
setValidity,
}: AggParamEditorProps<SelectValueProp> & SelectParamEditorProps) {
const label = i18n.translate('common.ui.aggTypes.orderLabel', {
defaultMessage: 'Order',
});

useEffect(
() => {
setValidity(!!value);
},
[value]
);

return (
<EuiFormRow
label={label}
fullWidth={true}
isInvalid={isInvalid}
className="visEditorSidebar__aggParamFormRow"
>
<EuiSelect
options={aggParam.options.raw}
value={value.value}
onChange={ev => setValue(aggParam.options.byValue[ev.target.value])}
fullWidth={true}
isInvalid={isInvalid}
/>
</EuiFormRow>
);
}

export { OrderParamEditor };
36 changes: 0 additions & 36 deletions src/legacy/ui/public/agg_types/controls/order_and_size.html

This file was deleted.

64 changes: 64 additions & 0 deletions src/legacy/ui/public/agg_types/controls/size.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
/*
* Licensed to Elasticsearch B.V. under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch B.V. licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

import React, { useEffect } from 'react';
import { isUndefined } from 'lodash';
import { AggParamEditorProps } from 'ui/vis/editors/default';
import { EuiFormRow, EuiFieldNumber } from '@elastic/eui';
import { i18n } from '@kbn/i18n';

function SizeParamEditor({
value,
setValue,
isInvalid,
setValidity,
setTouched,
}: AggParamEditorProps<number | ''>) {
const label = i18n.translate('common.ui.aggTypes.sizeLabel', {
defaultMessage: 'Size',
});

useEffect(
() => {
setValidity(Number(value) > 0);
},
[value]
);

return (
<EuiFormRow
label={label}
fullWidth={true}
isInvalid={isInvalid}
className="visEditorSidebar__aggParamFormRow"
>
<EuiFieldNumber
value={isUndefined(value) ? '' : value}
onChange={ev => setValue(ev.target.value === '' ? '' : parseFloat(ev.target.value))}
fullWidth={true}
isInvalid={isInvalid}
onBlur={setTouched}
min={1}
data-test-subj="sizeParamEditor"
/>
</EuiFormRow>
);
}

export { SizeParamEditor };
39 changes: 39 additions & 0 deletions src/legacy/ui/public/agg_types/param_types/select.d.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
/*
* Licensed to Elasticsearch B.V. under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch B.V. licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

import { IndexedArray } from '../../indexed_array';

interface SelectValueProp {
value: string;
text: string;
}

interface SelectOptions extends IndexedArray<SelectValueProp> {
byValue: {
[key: string]: SelectValueProp;
};
}

interface SelectParamEditorProps {
aggParam: {
options: SelectOptions;
};
}

export { SelectValueProp, SelectParamEditorProps };
Loading