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

Swap gem layout #2774

Merged
merged 9 commits into from
Jun 3, 2021
Merged

Swap gem layout #2774

merged 9 commits into from
Jun 3, 2021

Conversation

chris-gds
Copy link
Contributor

@chris-gds chris-gds commented May 25, 2021

What?

Update frontend app to use gem_layout template from static.

Why?

As part of on-going a11y work to align architecture & use Components to ensure consistency and future proof to allow a11y improvements to cascade across the site.

Visuals

Slight changes, some manual spacing adjustments have been made to mimic current design.

Selection of visuals:

Dev URL Before / After - Mobile Desktop
http://frontend.dev.gov.uk/ Screenshot 2021-05-31 at 10 52 48 Screenshot 2021-05-31 at 10 46 06
http://frontend.dev.gov.uk/foreign-travel-advice Screenshot 2021-05-31 at 12 40 13 Screenshot 2021-05-31 at 12 40 39
http://frontend.dev.gov.uk/contact-electoral-registration-office Screenshot 2021-05-31 at 13 45 50 Screenshot 2021-05-31 at 13 45 16
http://frontend.dev.gov.uk/help/cookies Screenshot 2021-05-31 at 14 15 38 Screenshot 2021-05-31 at 14 14 59
http://frontend.dev.gov.uk/settle-in-the-uk Screenshot 2021-05-31 at 14 19 05 Screenshot 2021-05-31 at 14 18 19
http://frontend.dev.gov.uk/settle-in-the-uk/y/you-re-an-eu-eea-or-swiss-citizen Screenshot 2021-05-31 at 16 46 31 Screenshot 2021-05-31 at 16 45 45

Anything else?

Related PRs

Enhancement PR:
Remove duplicate <headers>

Resolve e2e PR
alphagov/govuk_publishing_components#2101

Adding additional URLs to README for ref.
#2793


Converting the layout to the gem version resulted in the lookup pattern not inheriting the relevant styling correctly. Upon inspection, this is due to elements from the gem like fieldset not being utilised within frontend and therefore not using the shared classes - this has been manually adjusted.

Example page:

http://www.gov.uk/find-electoral-things

Screenshot 2021-05-31 at 15 06 18

Solution

This ties into the issue raised here:
alphagov/govuk_publishing_components#2096

Should the lookup pattern be it's own Component or use a common combination of Components (fieldset, input, button etc) these discrepancies would reduce.

@bevanloon bevanloon temporarily deployed to govuk-fronte-swap-gem-l-be1yab May 25, 2021 13:05 Inactive
@bevanloon bevanloon temporarily deployed to govuk-fronte-swap-gem-l-be1yab May 25, 2021 13:07 Inactive
@chris-gds chris-gds changed the title Swap gem layout reduced Swap gem layout May 25, 2021
@bevanloon bevanloon temporarily deployed to govuk-fronte-swap-gem-l-be1yab May 26, 2021 17:36 Inactive
@bevanloon bevanloon temporarily deployed to govuk-fronte-swap-gem-l-be1yab May 28, 2021 12:24 Inactive
Updating frontend to use the gem_layout
@bevanloon bevanloon temporarily deployed to govuk-fronte-swap-gem-l-be1yab May 28, 2021 13:11 Inactive
@bevanloon bevanloon temporarily deployed to govuk-fronte-swap-gem-l-be1yab May 28, 2021 13:25 Inactive
@bevanloon bevanloon temporarily deployed to govuk-fronte-swap-gem-l-be1yab May 28, 2021 16:11 Inactive
Changing some templates to use the full-width layout.
Change layout & dependancies

- Remove redundant feedback element (appeared twice on page)

- Example Page: /find-local-council
Update to resolve visual differences for frontend swap, changes resolve a border that is around the lookup element

Remove borders / padding from swap on:
"/foreign-travel-advice"
to maintain existing visuals.
@bevanloon bevanloon temporarily deployed to govuk-fronte-swap-gem-l-be1yab May 31, 2021 13:54 Inactive
to include Design System class
@bevanloon bevanloon temporarily deployed to govuk-fronte-swap-gem-l-be1yab May 31, 2021 15:52 Inactive
Chris Yoong added 5 commits May 31, 2021 17:10
+ add margin to replace spacing between lookup and feedback.
eg: /find-local-council
Adjust grid to match Design System + slight margin adjustments to mimic current design
- Update "/settle-in-the-uk" flow to use Design System grid, 
- Remove redundant CSS
- Adjust Design to maintain or align.
Redundant as gem_layout_full_width replaces this.
@bevanloon bevanloon temporarily deployed to govuk-fronte-swap-gem-l-be1yab May 31, 2021 16:19 Inactive
@chris-gds chris-gds marked this pull request as ready for review June 1, 2021 07:59
Copy link
Contributor

@owenatgov owenatgov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few pithy comments and questions. I think quite a few of these are classes used for testing only, which is something we don't wanna do so we don't clutter our markup. Incredible cleanup job here!

app/views/help/cookie_settings.html.erb Outdated Show resolved Hide resolved
app/views/place/_option_report_child_abuse.html.erb Outdated Show resolved Hide resolved
app/views/shared/_base_page.html.erb Outdated Show resolved Hide resolved
app/views/shared/_base_page.html.erb Outdated Show resolved Hide resolved
app/views/shared/_base_page.html.erb Show resolved Hide resolved
app/views/simple_smart_answers/flow.html.erb Show resolved Hide resolved
Update to remove redundant classes and tags
@bevanloon bevanloon temporarily deployed to govuk-fronte-swap-gem-l-be1yab June 1, 2021 16:27 Inactive
This was referenced Jun 1, 2021
Copy link
Contributor

@owenatgov owenatgov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unofficially approved 👍 I reckon we should hold off on this until Ian's point on the heading is sorted out.

@@ -69,7 +69,7 @@ class LicenceTest < ActionDispatch::IntegrationTest
end

within "#content" do
within ".page-header" do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Always big ❤️ to removing test-only classes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unofficially approved 👍 I reckon we should hold off on this until Ian's point on the heading is sorted out.

🙌🏼 Holding tight re the header visual.

@injms injms merged commit f80e3e7 into master Jun 3, 2021
@injms injms deleted the swap-gem-layout-reduced branch June 3, 2021 11:31
@injms injms restored the swap-gem-layout-reduced branch June 4, 2021 09:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants