Skip to content
This repository has been archived by the owner. It is now read-only.

Modernise markup and styles #21

Merged
merged 18 commits into from Feb 10, 2016
Merged

Modernise markup and styles #21

merged 18 commits into from Feb 10, 2016

Conversation

@fofr
Copy link
Contributor

@fofr fofr commented Feb 9, 2016

I recommend reviewing this commit by commit, and using ?w=1 to ignore whitespace (HTML indentation changes create noise). Having somebody check out the mobile/tablet views will also help.

Two critical changes;

  • Fix all IE8, IE7 and IE6 styles
  • Add missing help-notice styles so that FCO advice not to travel to a country is big and clear

Clean up:

  • Remove all legacy styles – most styles are provided by static and components
  • Update from legacy grid to new grid
  • Allow views to set a page class based on format, similar to government-frontend
  • Replace the grey summary/updated pattern with the metadata component
  • Use govspeak component for summary content
  • Stop using title component for part titles

Once alphagov/static#730 is deployed the last commit in this PR can be reverted.

https://trello.com/c/Lv5JExC0/283-multipage-frontend-cleanup-1-day

Country Summary

Before After
travel-advice-before travel-advice-after

Part

Before After
travel-advice-health-before travel-advice-health-after

Help notices

Before

screen shot 2016-02-09 at 12 34 19

After

screen shot 2016-02-09 at 12 49 50

Older Internet Explorer

IE before

screen shot 2016-02-09 at 14 25 53

IE after

screen shot 2016-02-09 at 14 35 45

cc @dsingleton @steventux

fofr added 17 commits Feb 10, 2016
* Remove all legacy styles ported across, begin from scratch and build
up
* Most styles are provided by static and components
* Remove toolkit imports until they’re needed
* Remove multipage – keep all styles travel_advice related while that’s
all the app serves, later frontend refactoring should be done when a
second format is added
* Remove #content, `content-block`, `inner`, `group` wrappers
* Use two thirds/one third grid layout
* Remove duplicate `role=main` attribute
Inner is an old grid class
Configure page_class as it is in government-frontend. Begin minimising
differences between these two frontends.
* Base on original frontend
* Remove text-align styles as they were redundant
* Remove mobile styles in favour of margins that use ems, these scale
automatically
* Style email and feed links
* Port existing styles but simplify
* Remove media queries, these styles look fine at all viewports
* Use `bold-19` mixin to set font size and weight consistently
* Port styles but simplify
* Use mobile first styles to reduce queries, switch to `core-19` at
thin viewports for larger navigation
The styles provided by the title component aren’t suitable here.
Instead use heading mixins to create the styles.

`part-title` class is already used, use `part-content-title` instead.
The `.column-one-third-desktop` should eventually be ported to static.
Putting here to avoid a static deploy dependency.
Simple user of gutter, but this should really be a re-usable pattern to
avoid margin values changing on title and the spacing looking incorrect.
Use an existing pattern rather than style and markup a slightly
different one.
Avoid custom content styles, instead use the govspeak component. The
schema defines this content as being type “govspeak”.
This feels like a component as it’s a cross app pattern, so should
eventually live in static.
Port from older multipage front-end styles, originally in static.
Content is not rendering correctly on older version of IE because these
styles aren’t being generated.
While alphagov/static#730 hasn’t been deployed,
add temporary styles so that these changes aren’t blocked.
@fofr fofr force-pushed the frontend-cleanup branch from b8b0b1e to aeb40c2 Feb 10, 2016
@jamiecobbett
Copy link

@jamiecobbett jamiecobbett commented Feb 10, 2016

In the screenshots, the summary has "Latest update: Latest update: ".

@fofr
Copy link
Contributor Author

@fofr fofr commented Feb 10, 2016

@jamiecobbett Good spot, it looks like it's hardcoded into the content item:

"change_description": "Latest update: Summary - there have been a significant number of attacks in Kabul in recent months, including on the Afghan news agency and military bases and convoys "

@binaryberry
Copy link

@binaryberry binaryberry commented Feb 10, 2016

At the moment in Chrome and Firefox, when reducing the window to minimum width, a horizontal bar appears towards the top, between the country name and summary. This change moves that bar down to after the heading links and "Get updates". Is that intentional?

@fofr
Copy link
Contributor Author

@fofr fofr commented Feb 10, 2016

The "Latest update" text is added by publishers:
screen shot 2016-02-10 at 14 54 07

@fofr
Copy link
Contributor Author

@fofr fofr commented Feb 10, 2016

@binaryberry That's a bug with the live version on smaller viewports – the floating items aren't being cleared correctly.

@dsingleton
Copy link
Contributor

@dsingleton dsingleton commented Feb 10, 2016

Overall LGTM 👍

Doubling "latest update" feels like a regression we shouldn't introduce. Could we strip that in the presenter, or finding a phrasing of the label that avoids the issue? Feels like an issue with the Publisher UI that editors are adding a "latest" prefix, but fixing that is outside the scope of this PR.

Otherwise happy to merge.

@binaryberry
Copy link

@binaryberry binaryberry commented Feb 10, 2016

@fofr , and this fixes the bug, got it!
LGTM too! I compared the old and new versions in Firefox and Chrome, Desktop (various window sizes) and various mobile devices (iPad, Nexus, Samsung, etc.). I see some differences, but they are improvements: changed related-items column size and darker metadata font colour improve readability, and the horizontal bar bug is fixed.

Change description is used as "Latest update" but isn't labelled that
way in the publisher. The frontend didn't add this label until now.

This leds to users appending (in a variety of formats) "Latest update:"
to the start of the change description.

The frontend now has a latest update label (to match usage), so we can
strip this out.

Avoids: "Latest update: Latest update - …"
@fofr
Copy link
Contributor Author

@fofr fofr commented Feb 10, 2016

Duplicate "Latest update" fixed in 9745dcd

dsingleton added a commit that referenced this issue Feb 10, 2016
@dsingleton dsingleton merged commit 069d76c into master Feb 10, 2016
1 check passed
@dsingleton dsingleton deleted the frontend-cleanup branch Feb 10, 2016
@dsingleton
Copy link
Contributor

@dsingleton dsingleton commented Feb 10, 2016

Merged as this is an overall improvement, and fixes a couple of production-affecting issues so we should :shipit: ASAP.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants