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

185 sort catalog properties #187

Merged
merged 10 commits into from
Dec 25, 2018
Merged

185 sort catalog properties #187

merged 10 commits into from
Dec 25, 2018

Conversation

@CLAassistant
Copy link

CLAassistant commented Dec 17, 2018

CLA assistant check
All committers have signed the CLA.

@@ -121,7 +121,7 @@ public GenericSearchResult<PropertyDictionaryItem> Search(PropertyDictionaryItem
var sortInfos = criteria.SortInfos;
if (sortInfos.IsNullOrEmpty())
{
sortInfos = new[] { new SortInfo { SortColumn = "SortOrder" } };
sortInfos = new[] { new SortInfo { SortColumn = "SortOrder", SortDirection = SortDirection.Descending } };
Copy link
Contributor

@artem-dudarev artem-dudarev Dec 17, 2018

Choose a reason for hiding this comment

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

This is incorrect.
Sort by SortOrder then by Alias

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added second sort info - by Alias

Copy link
Contributor

Choose a reason for hiding this comment

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

Not descending!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to ascending

Copy link
Contributor

@tatarincev tatarincev left a comment

Choose a reason for hiding this comment

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

Please read technical details for Issue more careful, you didn't reach the main goal (sort for catalog item property values according to SortOrder)

…y values by SortOrder parameter then by name/alias
Copy link
Contributor

@artem-dudarev artem-dudarev left a comment

Choose a reason for hiding this comment

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

Sorting by SortOrder should be ascending

@artem-dudarev
Copy link
Contributor

What about other 3 places?

@Andrew-Orlov
Copy link
Contributor Author

Andrew-Orlov commented Dec 18, 2018

I have told with Eugene and double checked this issue from the storefront. And how do you think - should it be described in the task requirements and not in the comments after? I mean sort direction, second sort criteria and so on.

@artem-dudarev
Copy link
Contributor

The issue description doesn't require to sort with descending order. So there was no need to introduce such sorting.

@tatarincev tatarincev changed the base branch from master to dev December 25, 2018 13:46
@tatarincev tatarincev merged commit 56fb799 into dev Dec 25, 2018
@tatarincev tatarincev deleted the 185-sort-catalog-properties branch December 25, 2018 13:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants