-
Notifications
You must be signed in to change notification settings - Fork 0
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
Create One Login bridging page #968
Conversation
…es to account page
Changes preview: |
app/helpers/application_helper.rb
Outdated
header.with_action_link(text: 'Sign in', href: new_user_session_path, options: { inverse: true }) | ||
header.with_navigation_item(text: 'Sign in', href: new_user_session_path, classes: %w[dfe-header__navigation-item dfe-header-f-mob]) | ||
header.with_action_link(text: 'Sign in', href: 'gov-one/info', options: { inverse: true }) | ||
header.with_navigation_item(text: 'Sign in', href: 'gov-one/info', classes: %w[dfe-header__navigation-item dfe-header-f-mob]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there not a path helper?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like this? 7b776be
app/views/home/index.html.slim
Outdated
.govuk-button-group | ||
= govuk_button_link_to 'gov-one/info', class: "govuk-button--start" do | ||
| #{t('home.gov-one-button')} | ||
svg.govuk-button__start-icon xmlns='http://www.w3.org/2000/svg' width='17.5' height='19' viewBox='0 0 33 40' aria-hidden='true' focusable='false' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Think I'd left a comment in the code about moving this CTA button into a partial. If you can manage it as part of this please do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
config/routes.rb
Outdated
@@ -5,7 +5,7 @@ | |||
|
|||
get 'my-modules', to: 'learning#show' # @see User#course | |||
get 'about-training', to: 'training/modules#index', as: :course_overview | |||
get 'gov-one/info', to: 'gov_one#info' | |||
get 'gov-one/info', to: 'gov_one#info', as: :gov_one_info |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiosity what name was it given before?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point! Not necessary
app/views/home/_cta.html.slim
Outdated
.govuk-button-group | ||
= govuk_button_link_to gov_one_info_path, class: "govuk-button--start" do | ||
| #{t('home.gov-one-button')} | ||
svg.govuk-button__start-icon xmlns='http://www.w3.org/2000/svg' width='17.5' height='19' viewBox='0 0 33 40' aria-hidden='true' focusable='false' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry just meant the SVG, see the module overview
app/views/gov_one/info.html.slim
Outdated
.govuk-grid-column-full class='govuk-!-margin-top-4 govuk-!-margin-bottom-9' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change the containing class to three-quarters
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
config/locales/en.yml
Outdated
@@ -575,9 +578,12 @@ en: | |||
# /gov-one/info | |||
gov_one_info: | |||
title: Gov One Info | |||
body: This is some information about Gov One Login. | |||
hero_header: How to access this training course |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because I18n has :scope
I like to nest keys where possible. Here that would create 2 keys under gov_one_info.hero
def info; end | ||
layout 'hero' | ||
|
||
def info |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think it is possible to stick to RESTful CRUD actions in this controller at this point in time?
config/locales/en.yml
Outdated
@@ -508,6 +508,8 @@ en: | |||
home: | |||
title: Home page | |||
hero: | | |||
# %{service_name} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haven't checked but I think this needs to be removed.
spec/system/gov_one_info_spec.rb
Outdated
end | ||
|
||
it 'has the correct login link' do | ||
link = find_link('Continue to GOV.UK One Login') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this complex link is asserted elsewhere I'd favour a simpler, more readable, spec.
@@ -31,11 +31,6 @@ def login_button | |||
govuk_button_link_to t('gov_one_info.sign_in_button'), login_uri.to_s | |||
end | |||
|
|||
# @return [String] | |||
def logout_button | |||
govuk_button_link_to t('gov_one_info.sign_out_button'), logout_uri.to_s |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we also removing the locale/resource? Again, if there are multiple button keys, we can nest them (flips how you read it).
@@ -11,20 +11,17 @@ | |||
.govuk-grid-column-one-half | |||
= m('home.about', headings_start_with: 'xl') | |||
|
|||
- unless current_user | |||
.govuk-grid-column-one-half | |||
.light-grey-box.enrol-box |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are .enrol-box
and .white-space-pre-wrap
being used elsewhere or can they also be deleted?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, deleted now
spec/system/whats_new_page_spec.rb
Outdated
@@ -20,18 +20,20 @@ | |||
end | |||
|
|||
describe 'with subsequent logins' do | |||
before do | |||
click_on 'sign-out-desktop' | |||
skip 'wip' do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is skipping necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed 948fa98
ER-875