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

Feature/catalogue toggle sort filter bar #1165

Merged
merged 69 commits into from
Jun 22, 2016

Conversation

brylie
Copy link
Contributor

@brylie brylie commented Jun 16, 2016

Closes #1127

@brylie brylie added this to the Sprint 24 milestone Jun 16, 2016
@@ -1,5 +1,5 @@
<template name="favourite">
{{#if isBookmarked}}
{{#if isBookmarked }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Add spaces here, e.g. {{ # if isBookmarked }}

@frenchbread
Copy link
Contributor

👍

@frenchbread frenchbread merged commit 4f3cfba into develop Jun 22, 2016
@frenchbread frenchbread deleted the feature/catalogue-toggle-sort-filter-bar branch June 22, 2016 12:51
@brylie
Copy link
Contributor Author

brylie commented Jun 27, 2016

What happened to the sorting feature here?

@@ -33,7 +33,7 @@ Meteor.publish('catalogue', function (options) {
};

// Set up query options with sort settings
queryOptions.sort[sortBy] = sortDirection;
Copy link
Contributor Author

@brylie brylie Jun 27, 2016

Choose a reason for hiding this comment

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

@NNN this syntax was probably necessary to make the sort feature work. In effect, we are trying to create an object key from a variable, so square bracket notation is required.

Reference

@frenchbread
Copy link
Contributor

@brylie If I'm recalling correctly, @NNN removed sorting functionality since we were not able to fix it and decided to hide it for now to be able to make a release.

// Subscribe to API Backends with catalogue settings
instance.subscribe("catalogue", subscriptionOptions);
instance.subscribe("catalogueRatings");
Copy link
Contributor Author

@brylie brylie Jun 27, 2016

Choose a reason for hiding this comment

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

The catalogueRatings subscription does not need to be inside of the autorun. Only the subscription that changes needs to be defined here. E.g. the catalogue subscription, since the subscription options change dynamically.

@brylie
Copy link
Contributor Author

brylie commented Jun 27, 2016

It looks like there was a regression added in the following commit:
apinf/api-umbrella-dashboard@5f77c9e

I.e. the code was switched from object bracket notation to dot notation. The bracket notation was important in this case.

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.

None yet

4 participants