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

Reverse actions array, so business actions come first #1714

Merged
merged 1 commit into from Nov 5, 2019

Conversation

@huwd
Copy link
Contributor

huwd commented Nov 4, 2019

Quick request by Mark and Stephen.
Would need to go live before PEP starts

I'm unsure if using .reverse like that is a terrible hack (currently there are only two audiences, not impossible there are more in future!).

Turns out the linter doesn't like me chaining end.reverse, gone for a slightly more verbose method though I'm still not sure if that's a terrible hack. A proper sort method would make sense, but I'm wondering if it's overkill?

@huwd huwd requested review from benthorner, emmabeynon and vanitabarrett Nov 4, 2019
@huwd huwd self-assigned this Nov 4, 2019
@bevanloon bevanloon temporarily deployed to finder-frontend-pr-1714 Nov 4, 2019 Inactive
@huwd huwd force-pushed the re-order-results branch from 71cf901 to 0f82491 Nov 4, 2019
@bevanloon bevanloon temporarily deployed to finder-frontend-pr-1714 Nov 4, 2019 Inactive
@@ -8,14 +8,16 @@ def format_criteria_list(criteria)
def format_action_audiences(actions)
action_groups = actions.group_by(&:audience)

action_groups.map do |key, action_group|
action_groups = action_groups.map do |key, action_group|

This comment has been minimized.

Copy link
@benthorner

benthorner Nov 5, 2019

Collaborator

It's hard to tell from reading this why we're reversing the groups. Can we do a sort_by here, so the ordering predicate is clear?

This comment has been minimized.

Copy link
@huwd

huwd Nov 5, 2019

Author Contributor

Went through with Jos and simplified massively. @benthorner hopefully the result is much easier to grok?

@huwd huwd force-pushed the re-order-results branch from 0f82491 to 8942f70 Nov 5, 2019
@bevanloon bevanloon temporarily deployed to finder-frontend-pr-1714 Nov 5, 2019 Inactive
Copy link
Collaborator

benthorner left a comment

Looks good to me - thanks for making it clearer 👍

@huwd huwd force-pushed the re-order-results branch from 8942f70 to 83d0e11 Nov 5, 2019
@huwd huwd merged commit 52f9f50 into master Nov 5, 2019
3 checks passed
3 checks passed
continuous-integration/jenkins/branch This commit looks good
Details
continuous-integration/jenkins/publishing-e2e-tests Publishing end-to-end tests succeeded on Jenkins
Details
continuous-integration/jenkins/security No security issues found
Details
@huwd huwd deleted the re-order-results branch Nov 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.