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

Switch to using govuk_admin_template and Bootstrap 3 #2241

Merged
merged 71 commits into from Jul 2, 2015
Merged

Conversation

@fofr
Copy link
Contributor

@fofr fofr commented Jun 23, 2015

Review using ?w=1 as many changes involved adding or removing HTML wrappers and altering indentation.

This is a large but also minimal pull request. I have aimed to keep it as small as possible – so that the majority looks and works exactly as it did before. The commits are predominantly class changes and the large bulk of it is:

  • switching from Bootstrap 2 to Bootstrap 3 grids
  • switching to Bootstrap 3 button classes
  • updating modals to new syntax
  • updating icon classes to use new glyphicons
  • switching to Bootstrap 3 form classes, styles are no longer applied automatically (mostly form-control and form-group)
  • updating alert- classes, mostly alert-error to alert-danger
  • minor typography improvements along the way
  • adding list-unstyled to remove Bootstrap 3’s default list styles where they aren't wanted

Some parts did require more attention and it's worth pointing those out:

  • The force publish modal on a link needed a shim to work with Bootstrap 3
  • The admin gem was updated so the Whitehall header could be used without significant changes
  • The words to avoid highlighter picked up some styles from the admin gem because of a class clash
  • Bootstrap 3 is more flexible in its use of horizontal form fields, but they now depend on grid classes, making a generic helper more complex. Horizontal form fields have been removed.
  • Bootstrap 3 doesn't support IE6, IE6 specific styles have been removed (4 sessions since January used IE6)
  • Use a Chosen Bootstrap 3 theme

All changes should only affect the admin parts of Whitehall. It's probably worth leaving this on Preview for a short while once we're happy with it, to try and spot anything that's been missed.

https://www.pivotaltracker.com/story/show/96072212

Depends on https://github.gds/gds/puppet/pull/2995

Before and after screenshots

New document

Before
screen shot 2015-06-23 at 16 20 05

After
screen shot 2015-06-23 at 16 19 50

Document list

Before
screen shot 2015-06-23 at 16 22 19

After
screen shot 2015-06-23 at 16 22 04

Dashboard

Before
screen shot 2015-06-23 at 16 27 10

After
screen shot 2015-06-23 at 16 26 58

Featured documents

Before
screen shot 2015-06-23 at 16 26 17

After
screen shot 2015-06-23 at 16 26 08

Collections

Before
screen shot 2015-06-23 at 16 25 28

After
screen shot 2015-06-23 at 16 25 14

Edit document

Before
screen shot 2015-06-23 at 16 24 51

After
screen shot 2015-06-23 at 16 24 39

Document summary

Before
screen shot 2015-06-23 at 16 23 42

After
screen shot 2015-06-23 at 16 23 31

@boffbowsh
Copy link
Contributor

@boffbowsh boffbowsh commented Jun 24, 2015

All the Ruby/Railsish stuff looks good to me 👍

Loading

@alicebartlett
Copy link
Contributor

@alicebartlett alicebartlett commented Jun 24, 2015

On the new document page, the govspeak helper box code examples no longer wrap. This page has a bunch of changes that I think are fine, this is the only one that we should probably fix as the helper is now quite difficult to use:

(before)
screen shot 2015-06-24 at 13 51 24

(after)
screen shot 2015-06-24 at 13 51 16

Loading

@fofr
Copy link
Contributor Author

@fofr fofr commented Jun 25, 2015

@alicebartlett I put back the missing pre styles (white-space: pre-wrap) so that lines would wrap correctly and be readable. This has introduced a new problem, where wrapping happens at a point that can cause confusion. In the example below it suggests that a line break is needed in link markup (I've seen this sort of thing cause problems in Whitehall training).

What are your thoughts?

Before
screen shot 2015-06-25 at 08 24 38

After
screen shot 2015-06-25 at 08 24 49

Loading

@alicebartlett
Copy link
Contributor

@alicebartlett alicebartlett commented Jun 25, 2015

@fofr I would defer to what the whitehall behaviour was before. In this case, the behaviour was to have the wrap, so the markdown looks like:

[a url]
(https://your-link-here.com)

or maybe even

[a url]
(https://your-link-
here.com)

which is what you have.

Loading

@fofr
Copy link
Contributor Author

@fofr fofr commented Jun 25, 2015

@alicebartlett Preview looks like:
screen shot 2015-06-25 at 09 39 40

Loading

@alicebartlett
Copy link
Contributor

@alicebartlett alicebartlett commented Jun 25, 2015

@fofr depending on your viewport width:
screen shot 2015-06-25 at 09 51 48

Loading

@alicebartlett
Copy link
Contributor

@alicebartlett alicebartlett commented Jun 25, 2015

OK, I'm done reviewing now. Happy for this to be merged. 👍

Loading

@fofr
Copy link
Contributor Author

@fofr fofr commented Jun 25, 2015

Thanks @alicebartlett! 😄

We're going to deploy this branch to preview for a few days to see if anything else surfaces.

Loading

@alicebartlett
Copy link
Contributor

@alicebartlett alicebartlett commented Jun 25, 2015

(p.s) Well done for getting this done, it was epic!

Loading

fofr added 21 commits Jun 26, 2015
* Add govuk_admin_template
* Remove jquery-rails and Bootstrap gems, they’re provided via gem
* Use nested layouts
* Remove bootstrap and custom variables from admin.scss
* Remove now unnecessary javascript includes
In Bootstrap 3 all form elements that want to pickup the Bootstrap
styles need:
* A `form-control` class
* A `form-group` wrapper for correct spacing

Horizontal forms no longer use `controls`, and the space between label
and field is set using newer grid classes. Setup horizontal forms for
text fields and textareas.
Environment indicators are now provided by the admin gem
These overly generic styles clash with the Bootstrap defaults and
patterns. Forms should look and behave as they do in other admin apps.
`error` was renamed to `danger`
* Switch to Bootstrap 3 grids where required
* Include Bootstrap 3 alert and btn default classes
* Switch featured link form away from horizontal styles, improve
rendering and capitalise `URL`
* Use styles and markup patterns from admin template, remove original
shims and assets
* Fix favicon image location
* Visually match existing header
* Use BS3 grid and label classes
* Use list-unstyled and list-inline classes and remove custom document
list styles
Pickup latest Bootstrap 3 templates
* Use BS3 grid, form and button classes
* Reduce custom styles in favour of Bootstrap defaults
* Remove default indenting within lists (`list-unstyled`)
* Add missing btn-default to buttons
* Tweak spacing
* Switch absolute_time to use a `<time>` element, and avoid styling as
an abbreviation with a help cursor and dotted underline
* Switch alerts from `-error` to `-danger`
* Small typography fixes
* Use no content styles for empty sections
* Switch from `icon-` to `glyphicon-`
* Update modals to latest syntax
* The radio/checkbox classes no longer go on the labels, they need to
wrap the label and the input. See http://getbootstrap.com/css/#forms
* Switch to new grid classes
* Fix checkboxes that aren’t using form_builder methods
* Update to work with admin gem’s approach to non-js checking
* Prevent `ul` elements from indenting list items
* Remove, simplify or update grid classes
* Use smart quotes in copy
* Add borders to tables to match table style
* Remove grid classes
* Put form in well to match other editing interfaces
* Use Bootstrap 3 radio classes
* Remove strong tags from label text
If `data-toggle=modal` and `data-target` are used on a link element in
Bootstrap 3, then Bootstrap attempts to load the contents of the link's
href remotely, putting it into the modal container. There's no way of
disabling this. (The feature is being removed in Bootstrap 4)

We still want a link that works when js is broken or disabled. Use
`data-module=linked-modal` rather than `data-toggle=modal`

* The workaround doesn’t work in conjunction with the confirm dialogue,
remove the dialogue as the edition still requires a reason before it
can be published
* Use glyphicons, link and margin helpers
* Remove custom styles which are now provided by Bootstrap/gem
* Remove custom styles, use Bootstrap defaults
  * Labels are now bold by default
* Wrap radio elements in `radio` container
* Reset styles that were being inherited from the admin template due to
a class clash.
* Stop using `badge` styles in alert
* Explicitly remove `hide` class from alert
fofr added 22 commits Jun 26, 2015
* Use Bootstrap 3 grid classes
* Use admin template table styles
* Add missing `btn-default` class
* Use Bootstrap 3 grid and form classes
* Stop using horizontal form styles
* Add missing `btn-default` classes
* Switch from `spanX` to `col-md-X`
* Remove spanX specific styles
In Bootstrap 3 `btn` buttons must also specify a style class,
`btn-default` is the grey one.
`btn-small` to `btn-sm`
`btn-mini` to `btn-xs`
`btn-large` to `btn-lg`
* Remove last use of `horizontal: true`
* Simplify form builder to avoid use of horizontal forms, keeping all
forms consistent and avoiding the more complex (but more flexible)
implementation of horizontal forms in Bootstrap 3, which uses grid
classes.
* Remove horizontal tests and update others to test for new syntax
The governments table already used the `.to_s(:govuk_date)` feature,
even though those date formats weren’t present without the admin gem.

Now the gem is included these dates render in a user friendly way and
the tests can be updated.
The javascript that searched for, cloned and updated the IDs of image
inputs was closely coupled with the markup structure. When the new
bootstrap form wrappers were added the javascript couldn’t find the
reference input.

* Use a js- class name to show that javascript is using the element
* Update from the old `delegate` method to the newer `on` one
* Use `find` over `children` to search more than one level deep
* Avoid `input:first`, be specific and use the new js- class name
`muted` has been replaced with `text-muted`
Replace `.js-enabled` with `.js` so that elements that depend on
javascript running adopt the same styles.

`js-enabled` was replaced with `.js` when the base admin template was
adopted. See:

https://github.com/alphagov/govuk_admin_template/blob/master/app/views/l
ayouts/govuk_admin_template.html.erb#L15

This only affects admin uses of `js-enabled`
* Remove indenting from lists using `list-unstyled`
* Switch to Bootstrap 3 button classes
* Use `no-content` styles for empty group
* Remove use of hgroup
* Use admin gem table styles
* Use Bootstrap 3 alert classes
* Add form classes to new import form
Bootstrap 3 changed the alert-error class to alert-danger
* Toggle `if-js-hide` class rather than show/hide
* Use Bootstrap 3 form classes
* Use Bootstrap 3 form classes
Update the js/non-js overrides to work with the new Bootstrap 3
collapse implementation.
* Use pre-built Bootstrap 3 chosen theme from:
http://github.com/alxlit/bootstrap-chosen
* Update `chzn-select` inputs to include a `form-control` class for
better non-js styling
Prevent users from having to sideways scroll to see content.
@fofr fofr force-pushed the govuk_admin_template branch from 186e0bb to 1519190 Jun 26, 2015
fofr added 2 commits Jun 29, 2015
* Remove Bootstrap 2 classes
* Switch to glyphicons
* Use no content styles from admin gem
Ensure the stylesheet includes the correct asset path when assets are
compiled.
@fofr fofr force-pushed the govuk_admin_template branch from 021e99c to 25295ee Jun 29, 2015
@mdo
Copy link

@mdo mdo commented Jun 30, 2015

😻

Loading

fofr added a commit that referenced this issue Jul 2, 2015
Switch to using govuk_admin_template and Bootstrap 3
@fofr fofr merged commit d2d0ee3 into master Jul 2, 2015
1 check passed
Loading
@fofr fofr deleted the govuk_admin_template branch Jul 2, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants