Remove legacy layout #2648
Remove legacy layout #2648
Conversation
chrislo
force-pushed
the
design-system-mop-up-v2
branch
9 times, most recently
from
ff029eb
to
7a387c5
Compare
There was a problem hiding this comment.
This makes me very happy! 🙌 Good work! 🎉
This isn't anything you've done, but when I was looking at the old layout code you removed, I noticed that in this commit where the 2SV prompt page was converted to use the Design System layout and in this commit where the new 2SV session page was converted to use the Design System layout, the suppress_navbar_items functionality was lost. I haven't looked into it, so I don't know how important that functionality was, but we should either (a) reinstate it; or (b) remove the relevant bits of code.
Similarly, in this commit where all the Devise::TwoStepVerificationSessionController templates are switched to use the GOV.UK Design System layout, the content_for(:thin_form) seems to have been accidentally left in the app/views/devise/two_step_verification_session/max_2sv_login_attempts_reached.html.erb template. I'm pretty sure it can just be removed.
Also are you planning to remove the govuk_admin_template gem and all it's associated stuff in a separate PR? I wonder if doing that might make more sense as part of this PR. What do you think...?
Anyway, I'm happy to mark this as approved and let you decide!
chrislo
force-pushed
the
design-system-mop-up-v2
branch
3 times, most recently
from
96b2eac
to
8500c21
Compare
|
Just looking at 8500c21, I'm a bit nervous that deleting Also related to the above, although unrelated to
Did you have any luck working out what to do about this? ☝️ |
|
Further to my point about Content Security Policy and given that we don't report on JS exceptions from production environments, I wonder if it might be sensible to deploy this branch to integration and check that a few things (like those Rails UJS links) are working and nothing is raising exceptions...? |
chrislo
force-pushed
the
design-system-mop-up-v2
branch
2 times, most recently
from
982123c
to
d19122e
Compare
|
After discussing with @floehopper I've removed the content security changes, we can see if they're still required and relevant if we decide to port the remaining uses of UJS to turbo (or remove them in favour of not using js at all) as part of the rails upgrade. There's still some places where we use |
|
Rebasing this on |
We want to move this error page to the design system. It was previously rendered by the error template provided by the doorkeeper gem[1]. This commit adds a copy of that template to our views so that it can be subsequently converted to the design system. [1] https://github.com/doorkeeper-gem/doorkeeper/blob/main/app/views/doorkeeper/authorizations/error.html.erb
This controller was previously using the legacy design to render its error message. This commit ensures that it uses the design system instead. I've decided to use an `error_alert` component here and keep the message and description from the original devise-provided template which we copied into our code in a previous commit.
We noticed that these routes, provided by devise, were being rendered using a devise template inside the new design-system layout. Rather than convert them to use the design system we think we can remove them altogether because our UserMailer#unlock_instructions email doesn't include a link to unlock_url (this is different to the default devise email for user unlocking[1]). It looks like[2] we've never allowed users to unlock their own accounts, but that we did plan to allow that at some point. For now, rather than have a potential unstyled page in the application, we'll remove these routes altogether. If we do decide to allow unlocking in the future we'll need to make sure the devise-provided views are either over-ridden or styled correctly. We've checked logit and can see no access to the `/devise/unlock` path in production in the last 30 days. Co-authored-by: Chris Roos <chris.roos@gofreerange.com> [1] https://github.com/heartcombo/devise/blob/e2242a95f3bb2e68ec0e9a064238ff7af6429545/app/views/devise/mailer/unlock_instructions.html.erb#L7C4-L7C4 [2] 5c70330
In each of these controllers the `only` array included all of the actions in the controller so the call to `only` is redundant and can be removed. Co-authored-by: Chris Roos <chris.roos@gofreerange.com>
We can move this call to `layout` to the ApplicationController to remove duplication from the controllers that inherit from it. Note the SigninRequiredAuthorizationsController retains a call to `layout` to set the `admin_layout` as it doesn't inherit from ApplicationController. The PasswordsController also has an explict reference to `admin_layout` in a call to `render`. Later we will be able to make `admin_layout` the default layout and remove those calls too. Co-authored-by: Chris Roos <chris.roos@gofreerange.com>
This layout is no longer used by the application so can be removed. We've also removed the dependent legacy stylesheets, javascript and the vendored `chosen` code as well as the shared bootstrap_flash_messages partial and its helper since these are no longer used elsewhere in the app. Note that since 5b0f80d we are implementing the same sanitization of tokens in URLs that was being performed in this layout by the `sensitive_query_parameters?` block. Co-authored-by: Chris Roos <chris.roos@gofreerange.com>
Rails looks for a layout named `application` by default so by renaming `admin_layout` we can avoid explicit calls to `layout` in the ApplicationController, SigninRequiredAuthorizationsController and PasswordsController.
This is no longer used. Co-authored-by: Chris Roos <chris.roos@gofreerange.com>
We're not using this gem since bccacf4.
Now that we are no longer using the old bootstrap design system the appearance of this page is improved by using an `error_alert` component. I've also removed the call to `content_for :thin_form` as that is no longer relevant with the new design system.
These date/time formats are currently provided by the govuk_admin_template gem[1]. We want to remove that gem since we're not using most of what it provides now we've moved to the new design system. I've copied the formats from the gem into our date_formats initializer so we can continue using them after the gem is removed. [1] https://github.com/alphagov/govuk_admin_template/blob/5fc0de2dd570e12b5d816261cbe4a4bc18752845/lib/govuk_admin_template/engine.rb#L16
With the google analytics code in the application layout[1] this call to `with_ga_enabled` is no longer needed in the integration tests that use it. Removing it means we no longer make a call to `GovukAdminTemplate.configure` which makes it easier to remove the govuk_admin_template gem. [1] added in 7fe1e64
We no longer need this gem as we aren't using the govuk_admin_template any more. I've read the setup instructions[1] for this (deprecated) gem and reversed the configuration mentioned in that README. [1] https://github.com/alphagov/govuk_admin_template/blob/5fc0de2dd570e12b5d816261cbe4a4bc18752845/README.md
chrislo
force-pushed
the
design-system-mop-up-v2
branch
from
d19122e
to
b3bcb4e
Compare
Trello: https://trello.com/c/kAZCL59w
This PR removes the old layout and related code now that the application has been migrated to the design system.
The first two commits migrate an error page to the design system that we'd previously missed (see screenshots below for the before and after). We remove an unused route provided by Devise so that we don't have to migrate it to the design system. We then switch to using the design system as the default layout in the application and remove dead code. Finally we remove two gems that are no longer used in the application.
Error page before
Error page after