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

Jump to bottom

Updated support for sorting of drop downs #265

Merged
merged 5 commits into from
Feb 21, 2018
Merged

Updated support for sorting of drop downs #265

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6d6663d
Updated support for sorting of drop downs
chalettu Feb 20, 2018
09b0cc2
Updated specs and tweaked syntax on sorting
chalettu Feb 21, 2018
406dd0d
updated tests
chalettu Feb 21, 2018
47367db
Fixed edge case for description texts starting with a number
chalettu Feb 21, 2018
0873a72
Clarified test case
chalettu Feb 21, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/dialog-user/components/dialog-user/dialogField.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,9 +77,9 @@ describe('Dialog field test', () => {

describe('#convertValuesToArray', () => {
it('converts a string of default values to an array', () => {
dialogCtrl.dialogField.default_value = '["one", "two"]'
dialogCtrl.dialogField.default_value = '["one", "two"]';
dialogCtrl.convertValuesToArray();
expect(dialogCtrl.dialogField.default_value).toEqual(['one', 'two'])
expect(dialogCtrl.dialogField.default_value).toEqual(['one', 'two']);
});
});
});
Expand Down
13 changes: 13 additions & 0 deletions src/dialog-user/services/dialogData.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,4 +144,17 @@ describe('DialogDataService test', () => {
const expectedSortedResult = [[5, 'C'], [0, 'B'], [2, 'A']];
expect(testSortedDescription).toEqual(expectedSortedResult);
});
it('should allow a numeric Description field to be sorted in a dropdown', () => {
const testDropDownDescription = {
values: [
[0, '0'],
Copy link
Contributor

@himdel himdel Feb 21, 2018 •
edited

Choose a reason for hiding this comment

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

Also, can you change the keys and values to something different (from each other)?

It is impossible to tell from this spec if this really works or if it just sorts by value instead :).

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, and also without an 11 or something like that, it doesn't actually tell us if those are sorted as numbers or as strings..

Copy link
Member

Choose a reason for hiding this comment

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

Also, we need to set the data.data_type to integer which is part of the original problem that javascript was sorting 1, 11, and 2 as if they were strings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually that wasn't the problem this PR addresses. This deals with a use case where a user would decide to actually use numbers in the description field and then decide they want to sort the dropdown by the description.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@eclarizio , I went ahead and updated one of the test cases to actually specify the data type. It wasn't directly related to this test but I added it in for clarity sake.

[5, '5'],
[2, '2']
],
options: { sort_by: 'description', sort_order: 'descending' }
};
const testSortedDescription = dialogData.updateFieldSortOrder(testDropDownDescription);
const expectedSortedResult = [[5, '5'], [2, '2'], [0, '0']];
expect(testSortedDescription).toEqual(expectedSortedResult);
});
});
29 changes: 22 additions & 7 deletions src/dialog-user/services/dialogData.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,9 @@ export default class DialogDataService {
if (option[0] === String(field.default_value)) {
field.selected = option;
}
if (field.data_type === 'integer') {
dropDownValues.push([parseInt(option[0], 10), option[1]]);
} else {
dropDownValues.push(option);
}
const value = (field.data_type === 'integer' ? parseInt(option[0], 10) : option[0]);
const description = (parseInt(option[1], 10) ? parseInt(option[1], 10) : option[1]);
Copy link
Contributor

@himdel himdel Feb 21, 2018 •
edited

Choose a reason for hiding this comment

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

Is this description used only for sorting?

Because if not, this breaks on any description which starts with a number. "3 horses" will become just 3

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Description is only used for sorting the drop down list.

Copy link
Contributor

@himdel himdel Feb 21, 2018 •
edited

Choose a reason for hiding this comment

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

Oh.. and maybe an _.isNaN?

Otherwise 0 will still get sorted last.. (because parseInt("0", 10) is 0 => falsy)

EDIT: you can also use Number.isNaN if you don't like lodash, it is shimmed by es6-shim so no browser compatibility issues

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ha, you beat me to it. I just added isNan in just now.

dropDownValues.push([value, description]);
}
field.values = dropDownValues;
field.values = this.updateFieldSortOrder(field);
Expand All @@ -48,8 +46,25 @@ export default class DialogDataService {
*
**/
private updateFieldSortOrder(data) {
let values = _.sortBy(data.values, data.options.sort_by === 'value' ? 0 : 1);
return data.options.sort_order === 'ascending' ? values : values.reverse();
const SORT_DESCRIPTION = 1;
const SORT_VALUE = 0;
const FIRST_OPTION = 0;
const VALUE = 0;
const sortBy = (data.options.sort_by === 'value' ? SORT_VALUE : SORT_DESCRIPTION);
let tempValues = [...data.values];
let defaultDropdownField = [];
// The following if deals with a empty default option if it exists
if (data.data_type === 'integer' && _.isNaN(tempValues[FIRST_OPTION][VALUE]) ||
_.isNull(tempValues[FIRST_OPTION][VALUE])) {
defaultDropdownField = tempValues.shift();
}
Copy link
Member

Choose a reason for hiding this comment

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

Should we just combine this else if and the if with an || since both lines 58 and 60 do the same thing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought about that when i reviewed my own code. I think I will try that but will put the or on a separate line

let values = _.sortBy(tempValues, sortBy);
const sortedValues = data.options.sort_order === 'ascending' ? values : values.reverse();
if (defaultDropdownField.length) {
sortedValues.unshift(defaultDropdownField);
}

return sortedValues;
}

/**
Expand Down