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

Several UI improvements #641

Merged
merged 4 commits into from Jan 6, 2020
Merged

Several UI improvements #641

merged 4 commits into from Jan 6, 2020

Conversation

@koetsier
Copy link
Contributor

koetsier commented Dec 18, 2019

@karlbaker02 karlbaker02 temporarily deployed to email-alert-frontend-pr-641 Dec 18, 2019 Inactive
@koetsier koetsier force-pushed the ui-improvements branch from fa7ac93 to 2431c1c Dec 18, 2019
@karlbaker02 karlbaker02 temporarily deployed to email-alert-frontend-pr-641 Dec 18, 2019 Inactive
@koetsier koetsier force-pushed the ui-improvements branch from 2431c1c to 4835d59 Dec 18, 2019
@karlbaker02 karlbaker02 temporarily deployed to email-alert-frontend-pr-641 Dec 18, 2019 Inactive
@koetsier koetsier force-pushed the ui-improvements branch from 4835d59 to 5c2d45d Dec 19, 2019
@karlbaker02 karlbaker02 temporarily deployed to email-alert-frontend-pr-641 Dec 19, 2019 Inactive
@koetsier koetsier force-pushed the ui-improvements branch from 5c2d45d to b42d563 Dec 19, 2019
@karlbaker02 karlbaker02 temporarily deployed to email-alert-frontend-pr-641 Dec 19, 2019 Inactive
@koetsier koetsier force-pushed the ui-improvements branch from b42d563 to a64a2d3 Dec 19, 2019
@karlbaker02 karlbaker02 temporarily deployed to email-alert-frontend-pr-641 Dec 19, 2019 Inactive
@koetsier koetsier force-pushed the ui-improvements branch from a64a2d3 to 90fce00 Dec 19, 2019
@karlbaker02 karlbaker02 temporarily deployed to email-alert-frontend-pr-641 Dec 19, 2019 Inactive
@koetsier koetsier force-pushed the ui-improvements branch from 90fce00 to 4ae7a00 Dec 23, 2019
@karlbaker02 karlbaker02 temporarily deployed to email-alert-frontend-pr-641 Dec 23, 2019 Inactive
@koetsier koetsier force-pushed the ui-improvements branch from 4ae7a00 to b352cea Dec 23, 2019

def new
assign_back_url

This comment has been minimized.

Copy link
@benthorner

benthorner Dec 30, 2019

Contributor

This is better than doing it everywhere! I'm finding the change a bit confusing, though, as here we're calling assign_back_url, whereas on line 34, we're assigning @back_url manually. Looking at the assign_back_url, I can see that each line is specific to one of the two places where it's called.

For simplicity and consistency, could we remove the assign_back_url method altogether?

This comment has been minimized.

Copy link
@koetsier

koetsier Jan 2, 2020

Author Contributor

good point. fixed

koetsier added 2 commits Dec 17, 2019
Change button text from ‘Next’ to ‘Continue’
Change button text from ‘Subscribe’ to ‘Continue’

Change topic styling from topic in inverted commas to topic with emphasis style but no inverted commas
@koetsier koetsier force-pushed the ui-improvements branch 2 times, most recently from 4ca76cf to 54d6b62 Jan 2, 2020
Copy link
Contributor

benthorner left a comment

Looks good to me. I've added a few minor comments but generally good to go 👍 .

def and_i_select_frequency_and_submit
stub_email_alert_api_sends_subscription_verification_email("person@somewhere.uk", "daily", @topic_id)
choose(option: "daily")
within "form.checklist-email-signup" do

This comment has been minimized.

Copy link
@benthorner

benthorner Jan 2, 2020

Contributor

Minor: could we just do click_on "Continue" or something? I guess this approach might be more robust, but on the other hand, I had to think more about what the code is doing.

when_i_start_signup_via_govuk
and_i_select_frequency_and_submit
and_i_fill_in_my_email
then_i_can_see_a_back_url_with_correct_parameters

This comment has been minimized.

Copy link
@benthorner

benthorner Jan 2, 2020

Contributor

Minor: it's a bit strange to read this in a (UI) feature test, since the user can't actually "see a back URL". How about "then_i_can_go_back_to_the_frequency_page" or similar?

@koetsier koetsier force-pushed the ui-improvements branch 4 times, most recently from fed831f to d250fb3 Jan 2, 2020
@koetsier koetsier force-pushed the ui-improvements branch 2 times, most recently from 0aa7e11 to 3866754 Jan 2, 2020
Include a back button and add a feature test.
@koetsier koetsier force-pushed the ui-improvements branch 4 times, most recently from 2c79a97 to 2c83c12 Jan 2, 2020
Change topic styling from topic in inverted commas to topic with emphasis style but no inverted commas
@koetsier koetsier force-pushed the ui-improvements branch from 2c83c12 to 5e969e3 Jan 6, 2020
@huwd
huwd approved these changes Jan 6, 2020
@koetsier koetsier merged commit 51c39c7 into master Jan 6, 2020
2 checks passed
2 checks passed
continuous-integration/jenkins/branch This commit looks good
Details
continuous-integration/jenkins/security No security issues found
Details
@koetsier koetsier deleted the ui-improvements branch Jan 6, 2020
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.