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.

Sign up for GitHub

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

Parameter feedback - #5 Unsaved indication #4322

Merged
merged 6 commits into from
Jul 21, 2023
Merged

Parameter feedback - #5 Unsaved indication #4322

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8d0a986
Parameter feedback - #5 Unsaved indication
ranbena Oct 30, 2019
a1c2c52
Added ANGULAR_REMOVE_ME
ranbena Oct 31, 2019
c3d2217
Added cypress test
ranbena Oct 31, 2019
1c92bb2
Fixed percy screenshot
ranbena Oct 31, 2019
0f90fb2
Some code improvements
ranbena Nov 4, 2019
56f4947
Parameter input feedback - #6 Better value normalization (#4327)
ranbena Jul 21, 2023
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
10 changes: 4 additions & 6 deletions client/app/components/ParameterValueInput.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import Input from 'antd/lib/input';
import InputNumber from 'antd/lib/input-number';
import DateParameter from '@/components/dynamic-parameters/DateParameter';
import DateRangeParameter from '@/components/dynamic-parameters/DateRangeParameter';
import { isEqual } from 'lodash';
import { isEqual, trim } from 'lodash';
import { QueryBasedParameterInput } from './QueryBasedParameterInput';

import './ParameterValueInput.less';
Expand Down Expand Up @@ -59,7 +59,7 @@ class ParameterValueInput extends React.Component {
}

onSelect = (value) => {
const isDirty = !isEqual(value, this.props.value);
const isDirty = !isEqual(trim(value), trim(this.props.value));
this.setState({ value, isDirty });
this.props.onSelect(value, isDirty);
}
Expand Down Expand Up @@ -140,13 +140,11 @@ class ParameterValueInput extends React.Component {
const { className } = this.props;
const { value } = this.state;

const normalize = val => (isNaN(val) ? undefined : val);

return (
<InputNumber
className={className}
value={normalize(value)}
onChange={val => this.onSelect(normalize(val))}
value={value}
onChange={val => this.onSelect(val)}
/>
);
}
Expand Down
23 changes: 20 additions & 3 deletions client/app/components/Parameters.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import React from 'react';
import PropTypes from 'prop-types';
import { size, filter, forEach, extend, get } from 'lodash';
import { size, filter, forEach, extend, get, includes } from 'lodash';
import { react2angular } from 'react2angular';
import { SortableContainer, SortableElement, DragHandle } from '@/components/sortable';
import { $location } from '@/services/ng';
Expand Down Expand Up @@ -34,6 +34,7 @@ export class Parameters extends React.Component {
queryResultErrorData: PropTypes.shape({
parameters: PropTypes.objectOf(PropTypes.string),
}),
unsavedParameters: PropTypes.arrayOf(PropTypes.string),
};

static defaultProps = {
Expand All @@ -44,6 +45,7 @@ export class Parameters extends React.Component {
onPendingValuesChange: () => {},
onParametersEdit: () => {},
queryResultErrorData: {},
unsavedParameters: null,
};

constructor(props) {
Expand Down Expand Up @@ -140,7 +142,22 @@ export class Parameters extends React.Component {
const { queryResultErrorData } = this.props;
const error = get(queryResultErrorData, ['parameters', param.name], false);
if (error) {
return [error, 'error'];
const feedback = <Tooltip title={error}>{error}</Tooltip>;
return [feedback, 'error'];
}

// unsaved
const { unsavedParameters } = this.props;
if (includes(unsavedParameters, param.name)) {
const feedback = (
<>
Unsaved{' '}
<Tooltip title='Click the "Save" button to preserve this parameter.'>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@susodapop any better suggestions for this text?

Copy link
Contributor

Choose a reason for hiding this comment

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

'Click the "Save" button to make this the default value for this parameter.'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But it's actually for the parameter itself - not its value.
The case is when a new parameter is typed into the query but ain't saved to DB till the user hits the "save" button.

Copy link
Contributor

@susodapop susodapop Nov 4, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

In that case it's no different than any other change to a query. Is a special message necessary? I like the yellow outline. Perhaps the effect could be complemented by a similar change in color to the Save button.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not a must, but the asterisk indication imo is not clear enough for changes that are represented below it (including parameter setting changes which is out of scope).
I encourage you to experience it and see for your self if it provides a better user experience.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree the asterisk is sub-optimal. Could we replace it with an indicator that is more obvious?

If you keep the tooltip then the text you've written above is fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One way to go about it is not show or disable the Save button when query isn't dirty.
So when it is, there would be a clear indication that there are changes to be saved.

Another reason I wanted this inline indication was so that it would be clear which parameter corresponds to the one added. So "Unsaved" could be substituted with "New parameter" with a green color instead.

@arikfr how do you feel about this feature?

<i className="fa fa-question-circle" />
</Tooltip>
</>
);
return [feedback, 'warning'];
}

return [];
Expand Down Expand Up @@ -172,7 +189,7 @@ export class Parameters extends React.Component {
</div>
<Form.Item
validateStatus={touched ? '' : status}
help={feedback ? <Tooltip title={feedback}>{feedback}</Tooltip> : null}
help={feedback || null}
>
<ParameterValueInput
type={param.type}
Expand Down
2 changes: 1 addition & 1 deletion client/app/pages/queries/query.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,7 @@ <h3>
<div class="d-flex flex-column p-b-15 p-absolute static-position__mobile" style="left: 0; top: 0; right: 0; bottom: 0;">
<div class="p-t-15 p-b-5" ng-if="query.hasParameters()">
<parameters parameters="query.getParametersDefs()" query-result-error-data="queryResult.getErrorData()" editable="sourceMode && canEdit" disable-url-update="query.isNew()"
on-values-change="executeQuery" on-pending-values-change="applyParametersChanges" on-parameters-edit="onParametersUpdated"></parameters>
on-values-change="executeQuery" on-pending-values-change="applyParametersChanges" on-parameters-edit="onParametersUpdated" unsaved-parameters="getUnsavedParameters()"></parameters>
</div>
<!-- Query Execution Status -->

Expand Down
18 changes: 17 additions & 1 deletion client/app/pages/queries/source-view.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { map, debounce } from 'lodash';
import { map, debounce, isEmpty, isEqual } from 'lodash';
import template from './query.html';
import EditParameterSettingsDialog from '@/components/EditParameterSettingsDialog';

Expand Down Expand Up @@ -109,6 +109,22 @@ function QuerySourceCtrl(
$scope.$watch('query.query', (newQueryText) => {
$scope.isDirty = newQueryText !== queryText;
});

$scope.unsavedParameters = null;
$scope.getUnsavedParameters = () => {
if (!$scope.isDirty || !queryText) {
return null;
}
const unsavedParameters = $scope.query.$parameters.getUnsavedParameters(queryText);
if (isEmpty(unsavedParameters)) {
return null;
}
// avoiding Angular infdig (ANGULAR_REMOVE_ME)
if (!isEqual(unsavedParameters, $scope.unsavedParameters)) {
$scope.unsavedParameters = unsavedParameters;
}
return $scope.unsavedParameters;
};
}

export default function init(ngModule) {
Expand Down
6 changes: 3 additions & 3 deletions client/app/services/parameters/NumberParameter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { toNumber, isNull } from 'lodash';
import { toNumber, trim } from 'lodash';
import { Parameter } from '.';

class NumberParameter extends Parameter {
Expand All @@ -9,11 +9,11 @@ class NumberParameter extends Parameter {

// eslint-disable-next-line class-methods-use-this
normalizeValue(value) {
if (isNull(value)) {
if (!trim(value)) {
return null;
}
const normalizedValue = toNumber(value);
return !isNaN(normalizedValue) ? normalizedValue : null;
return !isNaN(normalizedValue) ? normalizedValue : value;
}
}

Expand Down
9 changes: 8 additions & 1 deletion client/app/services/parameters/TextParameter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { toString, isEmpty } from 'lodash';
import { toString, isEmpty, trim } from 'lodash';
import { Parameter } from '.';

class TextParameter extends Parameter {
Expand All @@ -15,6 +15,13 @@ class TextParameter extends Parameter {
}
return normalizedValue;
}

getExecutionValue() {
if (!trim(this.value)) {
return null;
}
return this.value;
}
}

export default TextParameter;
5 changes: 0 additions & 5 deletions client/app/services/parameters/tests/NumberParameter.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,5 @@ describe('NumberParameter', () => {
const normalizedValue = param.normalizeValue(42);
expect(normalizedValue).toBe(42);
});

test('returns null when not possible to convert to number', () => {
const normalizedValue = param.normalizeValue('notanumber');
expect(normalizedValue).toBeNull();
});
});
});
15 changes: 10 additions & 5 deletions client/app/services/query.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@ import moment from 'moment';
import debug from 'debug';
import Mustache from 'mustache';
import {
zipObject, isEmpty, map, includes, union,
uniq, has, identity, extend, each, some,
zipObject, isEmpty, map, includes, union, isNil,
uniq, has, identity, extend, each, some, reject,
} from 'lodash';

import { Parameter } from './parameters';
Expand Down Expand Up @@ -35,13 +35,13 @@ class Parameters {
this.initFromQueryString(queryString);
}

parseQuery() {
parseQuery(queryText = this.query.query) {
const fallback = () => map(this.query.options.parameters, i => i.name);

let parameters = [];
if (this.query.query !== undefined) {
if (!isNil(queryText)) {
try {
const parts = Mustache.parse(this.query.query);
const parts = Mustache.parse(queryText);
parameters = uniq(collectParams(parts));
} catch (e) {
logger('Failed parsing parameters: ', e);
Expand Down Expand Up @@ -124,6 +124,11 @@ class Parameters {
each(this.get(), p => p.applyPendingValue());
}

getUnsavedParameters(queryText) {
const savedParameters = this.parseQuery(queryText);
return reject(this.get(), p => includes(savedParameters, p.name)).map(p => p.name);
}

toUrlParams() {
if (this.get().length === 0) {
return '';
Expand Down
23 changes: 23 additions & 0 deletions client/cypress/integration/query/parameter_spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -614,6 +614,29 @@ describe('Parameter', () => {
cy.percySnapshot('Validation error in query page');
});

it('shows unsaved feedback in query page', function () {
cy.visit(`/queries/${this.query.id}/source`);

cy.getByTestId('QueryEditor')
.get('.ace_text-input')
.type(' {{ newparam }}', { force: true, parseSpecialCharSequences: false });

cy.getByTestId('ParameterName-newparam')
.find('.ant-form-item-control')
.should('have.class', 'has-warning')
.find('.ant-form-explain')
.as('Feedback');

cy.get('@Feedback')
.should('contain.text', 'Unsaved')
.should('not.have.class', 'show-help-appear'); // assures ant animation ended for screenshot

cy.percySnapshot('Unsaved feedback in query page');

cy.getByTestId('SaveButton').click();
cy.get('@Feedback').should('not.exist');
});

it('shows validation error in visualization embed', function () {
cy.visit(`/embed/query/${this.query.id}/visualization/${this.vizId}?api_key=${this.query.api_key}`);
expectValueValidationError();
Expand Down