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

Standardise margins on components #730

Merged
merged 9 commits into from Feb 11, 2016
Merged

Standardise margins on components #730

merged 9 commits into from Feb 11, 2016

Conversation

@fofr
Copy link
Contributor

@fofr fofr commented Feb 8, 2016

This work follows the principles:

  • Include a sensible default margin so that pages can be built quickly with the same vertical spacing
  • Removing a margin is easier than adding one
  • A sensible default usually looks like a consistent bottom margin
  • Top margin should be used sparingly. A good exception is the title component.

Changes to components:

  • Remove an unused margin styles from the previous and next component (two apps use it, one was unnecessary, the other was overriding)
  • Use responsive top and bottom margins on title, increasing spacing from 30px to 45px
  • Add default bottom margin to document footer – this is not responsive as the larger space is useful in separating footer from content at thinner page widths. Once deployed the app specific style in government-frontend can be removed.
  • Add bottom margin to option-selects, matching the styles applied in finder-frontend
  • Add default bottom margin to metadata component – dependent on alphagov/specialist-frontend#108

Other changes:

  • Fix left/right margin of report a problem wrapper when indented within #wrapper

https://trello.com/c/zwYvoQxY/273-standardise-whitespace-margins-on-components-medium

This story has highlighted a need for Sass linting, so that styles are written consistently across apps.

cc @dsingleton

fofr added 6 commits Feb 8, 2016
Zero is zero in all units.
As part of component standardising we don’t want the outer component
container to have top margins.

This component is used in finder frontend and manuals. In finder
frontend the style is overwritten by app specific styles, in
manuals it's not necessary.
Title components can keep their top and bottom margins, as they are
sensible defaults to use when dropping the component into a front-end.
Document footers appear in a consistent place on a page. Add a default
bottom margin so that apps do not have to add manual styles to separate
it from the “report a problem” link.

Currently only used by specialist-frontend and government-frontend.
Give option selects a margin so that each app using them doesn’t need
to provide them itself.

Base on value used in finder-fronted:
https://github.com/alphagov/finder-frontend/blob/09dba3aa9b7b4633932b71b
9593302c7d48f30f0/app/assets/stylesheets/finder_frontend.scss#L155
* Nav appears beneath content by default, a top margin makes sense in
this case
* Add a bottom margin by default, something will always follow it
* DRY up some of the styles

Margin use case:
https://github.com/alphagov/finder-frontend/blob/09dba3aa9b7b4633932b71b
9593302c7d48f30f0/app/assets/stylesheets/finder_frontend.scss#L347
fofr added a commit to alphagov/multipage-frontend that referenced this pull request Feb 9, 2016
While alphagov/static#730 hasn’t been deployed,
add temporary styles so that these changes aren’t blocked.
fofr added a commit to alphagov/multipage-frontend that referenced this pull request Feb 10, 2016
While alphagov/static#730 hasn’t been deployed,
add temporary styles so that these changes aren’t blocked.
@fofr fofr changed the title [DO NOT MERGE] Standardise margins on components Standardise margins on components Feb 10, 2016
@dsingleton
Copy link
Contributor

@dsingleton dsingleton commented Feb 10, 2016

Overall 👍

How did you test the changes and how confident are we there aren't any unintended changes? (I tried to fix report-a-problem alignment in the past and managed to break an edge case 😞)

We discussed top-margin on title offline and it's a reasonable one to have top margin, but i'm less sure on pagination. It's only used in two apps and feels like it might be easier to set in those two apps.

@fofr
Copy link
Contributor Author

@fofr fofr commented Feb 11, 2016

@dsingleton I based the top-margin decision for page nav component on the basis that both apps using it were adding a top margin to it, despite slightly different contexts. When more things use that component we can re-assess what the default should be.

@fofr
Copy link
Contributor Author

@fofr fofr commented Feb 11, 2016

@dsingleton Regarding "Report a problem", when that markup is in #wrapper, then wrapper is providing all of the page positioning and margins, in that instance it doesn't need any side margins. Tested on specialist-frontend, finder-frontend, government-frontend and frontend.

@fofr
Copy link
Contributor Author

@fofr fofr commented Feb 11, 2016

@dsingleton I tested each of the components in the context with which they are being used. A final check on integration once merged would be sensible.

fofr added 3 commits Feb 8, 2016
Ported from government-frontend.

* Provide a consistent way for components to set a responsive bottom
margin
Metadata component should have a default margin as a different type of
content usually follows. Follow the government-frontend example and use
a responsive margin.

Component is also used by specialist-frontend and finder-frontend.
Specialist front-end needs to be updated before this change is applied.
This increases the space above and below the title component by 15px to
match the `1.5 * gutter` used on the metadata and document footer
components.

Based on feedback from @markhurrell.
@fofr fofr force-pushed the no-component-margins branch to 6c74768 Feb 11, 2016
dsingleton added a commit that referenced this pull request Feb 11, 2016
Standardise margins on components
@dsingleton dsingleton merged commit 23dc3fe into master Feb 11, 2016
1 check passed
1 check passed
default "Build #833 succeeded on Jenkins"
Details
@dsingleton dsingleton deleted the no-component-margins branch Feb 11, 2016
@fofr
Copy link
Contributor Author

@fofr fofr commented Feb 15, 2016

This has been deployed, unblocking alphagov/government-frontend#93 and alphagov/specialist-frontend#109

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

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.