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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DO NOT MERGE] Add Subgroups to the results page #1731

Open
wants to merge 18 commits into
base: master
from

Conversation

@huwd
Copy link
Contributor

huwd commented Nov 8, 2019

Trello: https://trello.com/c/uLLaEVPV/186-dont-deploy-update-checker-results-page

PEP restricted PR

馃毃 Do not merge until product has approved. 馃毃

Background

The Brexit checker can return a large number of results, this can be overwhelming for the user.
For citizens actions this experiments with a newly grouped interface that attempts to make individual/family actions easier to parse.

What's involved

Actions and criteria were previously passed to the frontend as two separate variables. Both were arrays that could be ordered and then chucked into the frontend.

This PR makes that setup more complicated. An array of "audiences" (Business/Individual) are provided, with Business ordered first.

An audience has a heading, and groups. Groups then optionally have their own heading, actions and relevant criteria. The frontend now uses this structure to build the view.

Criteria do not come with any prior grouping. To obtain them for a given group we start with the results criteria, representing the User's choices and stored in the URL. We then extract the criteria from all actions in a group. Flatten these and use that to select relevant criteria from the page.

What it looks like

Current view on live:
Screenshot 2019-11-12 at 14 57 00

New view in this PR:
Screenshot 2019-11-12 at 14 58 01

Example links

@markwoods37 / @trevor_saint I've got this up here:

@bevanloon bevanloon temporarily deployed to finder-frontend-pr-1731 Nov 8, 2019 Inactive
@huwd huwd force-pushed the groupActionsByResults branch from 2ee0db5 to 3bd59da Nov 8, 2019
@bevanloon bevanloon temporarily deployed to finder-frontend-pr-1731 Nov 8, 2019 Inactive
@huwd huwd changed the title [DO NOT MERGE] Add citizen groupings to results page [DO NOT MERGE] Add Subgroups to the results page Nov 8, 2019
@huwd huwd force-pushed the groupActionsByResults branch from 3bd59da to 88195a4 Nov 8, 2019
@bevanloon bevanloon temporarily deployed to finder-frontend-pr-1731 Nov 8, 2019 Inactive
huwd added 2 commits Nov 11, 2019
Tests fail if it's a <p> tag instead of <div>, view layers remain unchanged when it is either
Updated to match newer choice of assistance pet, avoid travel business question
@bevanloon bevanloon temporarily deployed to finder-frontend-pr-1731 Nov 11, 2019 Inactive
There's only a single test here anyway, so it doesn't dry out much, and there's an irregular test failure reporting 'No such file or directory @ rb_sysopen - /tmp/downloaded.csv20191111-37-uz5lkw', this is an attempt to resolve any ordering issues
@huwd huwd force-pushed the groupActionsByResults branch from 0e1755f to 564275d Nov 11, 2019
@bevanloon bevanloon temporarily deployed to finder-frontend-pr-1731 Nov 11, 2019 Inactive
@bevanloon bevanloon temporarily deployed to finder-frontend-pr-1731 Nov 11, 2019 Inactive
@bevanloon bevanloon temporarily deployed to finder-frontend-pr-1731 Nov 11, 2019 Inactive
@huwd huwd force-pushed the groupActionsByResults branch from bdf382b to fbdd8ef Nov 12, 2019
@bevanloon bevanloon temporarily deployed to finder-frontend-pr-1731 Nov 12, 2019 Inactive
@huwd huwd requested review from benthorner, emmabeynon and koetsier Nov 12, 2019
<section class="brexit-checker-actions__group">
<div class="govuk-grid-row">
<div class="govuk-grid-column-one-third">
<%= render 'results_answers', answers: group[:criteria], heading: group[:heading] %>

This comment has been minimized.

Copy link
@koetsier

koetsier Nov 12, 2019

Contributor

not sure about renaming criteria to answers. Why not leave them criteria - less to remember

This comment has been minimized.

Copy link
@huwd

huwd Nov 13, 2019

Author Contributor

good point, have renamed this everywhere that previously referred to 'answers'

<div class="govuk-grid-column-two-thirds">
<%= render "action_list", {
audience_heading: group[:heading],
audience_index: group[:priority],

This comment has been minimized.

Copy link
@koetsier

koetsier Nov 12, 2019

Contributor

priority->index, might as well leave it priority

This comment has been minimized.

Copy link
@huwd

huwd Nov 13, 2019

Author Contributor

good shout, amended

<%= render "action_list", {
audience_heading: group[:heading],
audience_index: group[:priority],
items: group[:actions]

This comment has been minimized.

Copy link
@koetsier

koetsier Nov 12, 2019

Contributor

see above

This comment has been minimized.

Copy link
@huwd

huwd Nov 13, 2019

Author Contributor

馃憤 cheers, amended


def format_citizen_groups(actions, criteria)
all_groups = actions.map(&:groups).flatten.uniq
citizen_groups = all_groups.map do |group|

This comment has been minimized.

Copy link
@koetsier

koetsier Nov 12, 2019

Contributor

I would be tempted to use group_by on actions here. That converts it into a hash with the group name as key and lists of actions as keys. That would avoid the repetitive select in format_citizen_actions

This comment has been minimized.

Copy link
@huwd

huwd Nov 13, 2019

Author Contributor

Lets run through all this grouping flow?

end

def order_citizen_groups(citizen_groups)
citizen_groups.sort_by { |group| group[:priority] }.reverse

This comment has been minimized.

Copy link
@koetsier

koetsier Nov 12, 2019

Contributor

could use -group[:priority] to omit the reverse

<%= prompt %>:
<div class="govuk-body govuk-!-font-size-16">
<p>
<% prompt = action.guidance_prompt || t("brexit_checker.action_list.guidance_prompt") %>

This comment has been minimized.

Copy link
@koetsier

koetsier Nov 12, 2019

Contributor

could you not simply use:

<%= action.guidance_prompt || t("brexit_checker.action_list.guidance_prompt") %>

This comment has been minimized.

Copy link
@huwd

huwd Nov 13, 2019

Author Contributor

... doh... good catch


def initialize(attrs)
attrs.each { |key, value| instance_variable_set("@#{key}", value) }
validate!
load_groups

This comment has been minimized.

Copy link
@koetsier

koetsier Nov 12, 2019

Contributor

I'm not sure about merging in the groups here.

This is an action and should not really know about groups. It is also a bit confusing to have a variable called 'groups' and 'result_groups'. It is not immediately clear which-is-which

This comment has been minimized.

Copy link
@huwd

huwd Nov 13, 2019

Author Contributor

So i think my theory was, if I could load groups here.
Then each action goes from having just a results_group array of strings, to an array of proper rich groups, which have a priority key.

Allows me to do this here:
all_groups = actions.map(&:groups).flatten.uniq


attr_reader :key, :text, :priority

def initialize(attrs)

This comment has been minimized.

Copy link
@koetsier

koetsier Nov 12, 2019

Contributor

I'm wondering if it makes sense to store the groups as a hash of key->{text, priority}

This comment has been minimized.

Copy link
@huwd

huwd Nov 13, 2019

Author Contributor

Lets run through this?

private

def load_groups
all_groups ||= YAML.load_file(GROUPS_PATH)["groups"]

This comment has been minimized.

Copy link
@koetsier

koetsier Nov 12, 2019

Contributor

memoizing this does not work because it all_groups is a local variable

This comment has been minimized.

Copy link
@huwd

huwd Nov 13, 2019

Author Contributor

ah good point, humm didn't like the idea of loading these each time, could I use @@ here?
Looks like we should have a chat about this whole flow

@huwd huwd force-pushed the groupActionsByResults branch from 7cd3e3c to 98ce7c3 Nov 13, 2019
@bevanloon bevanloon temporarily deployed to finder-frontend-pr-1731 Nov 13, 2019 Inactive
@bevanloon bevanloon temporarily deployed to finder-frontend-pr-1731 Nov 13, 2019 Inactive
@huwd huwd force-pushed the groupActionsByResults branch from 9d211cb to fe4911b Nov 13, 2019
@bevanloon bevanloon temporarily deployed to finder-frontend-pr-1731 Nov 13, 2019 Inactive
@huwd huwd force-pushed the groupActionsByResults branch from fe4911b to 23fb65a Nov 13, 2019
@bevanloon bevanloon temporarily deployed to finder-frontend-pr-1731 Nov 13, 2019 Inactive
@huwd

This comment has been minimized.

Copy link
Contributor Author

huwd commented Nov 13, 2019

Is there meant to be a border-top on the email subscription section?

Screen Shot 2019-11-13 at 09 58 12

There is! I put that off in a separate ticket, I thought there's be more, but turns out it is just this.

@huwd huwd force-pushed the groupActionsByResults branch from 23fb65a to b82231b Nov 13, 2019
@bevanloon bevanloon temporarily deployed to finder-frontend-pr-1731 Nov 13, 2019 Inactive
@huwd

This comment has been minimized.

Copy link
Contributor Author

huwd commented Nov 13, 2019

Looks like there are two borders here (one grey, one blue):

Screen Shot 2019-11-13 at 10 03 02

Great, looks like that's fixed now

@huwd

This comment has been minimized.

Copy link
Contributor Author

huwd commented Nov 13, 2019

Looks like the tracking has broken a bit. The events are firing correctly, but the eventAction is (for example):

eventAction: " 10.01 - Guidance"

when it should be something like:

eventAction: "eventAction: "Your business or organisation 1.01 - Guidance"

This was testing with the first guidance link on the page too, so feels weird for the number to be 10.[x] rather than 1.[x]

Note: this applies to guidance links AND action links

Cool, might want to check in on how we fix these

@huwd

This comment has been minimized.

Copy link
Contributor Author

huwd commented Nov 13, 2019

Spacing between subgroups seems a lot larger than in the prototype.

Prototype vs this PR:
Screen Shot 2019-11-13 at 10 01 36Screen Shot 2019-11-13 at 09 59 16

Fixed, thanks!

emmabeynon added 6 commits Nov 14, 2019
This is a bold title above the link.
We move this CSS out of the .app-c-email-link__link hierarchy as it's no longer
a link, and we remove changing the colour of the icon to match the link colour.
Update email signup component
@bevanloon bevanloon temporarily deployed to finder-frontend-pr-1731 Nov 14, 2019 Inactive
huwd added 2 commits Nov 15, 2019
Fix tracking on new results page
@bevanloon bevanloon temporarily deployed to finder-frontend-pr-1731 Nov 19, 2019 Inactive
huwd added 2 commits Nov 15, 2019
Change breadcrumbs and title to new copy
@bevanloon bevanloon temporarily deployed to finder-frontend-pr-1731 Nov 19, 2019 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can鈥檛 perform that action at this time.