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

Create a component guide based on components in an application #1

Merged
merged 21 commits into from Jul 21, 2017

Conversation

@fofr
Copy link
Contributor

@fofr fofr commented Jul 19, 2017

Part of:
https://trello.com/c/SD6M3IUF/12-3-create-a-component-guide-that-can-document-components-in-apps

Not all components should be in static. If they are used only in one application it is preferable to keep them close to the point of use.

Initial release:

See README.md for further details.

Still to do but can be separate PRs:

  • Syntax highlighting
  • Component guide styles should be components themselves
  • Further documentation on components and principles
  • Component previews that render just the component (ie no GOV.UK frame around it)

screen shot 2017-07-19 at 17 18 31
screen shot 2017-07-19 at 17 18 49

fofr added 15 commits Jul 5, 2017
* Add wrapper ID to allow slimmer to run insertion correctly
* Add sass, toolkit and slimmer dependencies
* Create index route, controller and view
Use models, logic and styles from
https://github.com/alphagov/govuk-component-guide. Docs structure will
be identical.

* Render a list of components based on docs in the
`app/views/components/docs` directory of the host app
* Display a component with a set of test fixtures based on those docs
* Include those partials in the view path
* Use breadcrumbs component for navigating the guide
* Use a less verbose component call for using a partial
* Use same configuration pattern as govuk_admin_template
Add webmock dependency and require webmock as it does not come with
Slimmer.

Issue raised here:
alphagov/slimmer#201
* It ought to work with the version a host app uses
* Require in engine
* Confirm that the component guide index loads
* Lint Ruby and Sass
* Fix linting errors
Default both to rails convention, “application”
* Give each page a unique title based on guide name, component and
fixture
* Give each page a breadcrumb that matches the guide name
* Use the guide name as the h1 on the index
@fofr fofr force-pushed the living-style-guide branch from 43611f5 to dd857ee Jul 19, 2017
@fofr fofr changed the title Create a component guide based on components in application Create a component guide based on components in an application Jul 19, 2017
@@ -2,7 +2,10 @@ PATH
remote: .

This comment has been minimized.

@tijmenb

tijmenb Jul 20, 2017
Contributor

The Gemfile.lock shouldn't be committed for gems.

This comment has been minimized.

@fofr

fofr Jul 20, 2017
Author Contributor

Thanks, removed in 32895dd

Copy link
Contributor

@andysellick andysellick left a comment

As requested, I've focussed on the documentation for this review.

README.md Outdated
* a unit test in `/test/components` - testing the component renders correctly based on parameters passed in

The component guide will include your application’s styles and scripts. It will include `application.scss` and `application.js` by default but this is [configurable](#configuration).

This comment has been minimized.

@andysellick

andysellick Jul 20, 2017
Contributor

Should there be a line or two in here about how to call/use a component? I guess it's already going to be in the component guide though.

This comment has been minimized.

@fofr

fofr Jul 20, 2017
Author Contributor

I think it's worth adding one or two lines.

README.md Outdated

## Installation
Add this line to your application's Gemfile:
Add this line to your application's Gemfile in the development and test groups:

This comment has been minimized.

@andysellick

andysellick Jul 20, 2017
Contributor

As someone new to this, I don't know what the "development and test groups" are, so I'd be stuck on this step.

unless Rails.env.production?
mount GovukPublishingComponents::Engine, at: "/component-guide"
end
```

This comment has been minimized.

@andysellick

andysellick Jul 20, 2017
Contributor

Is this a change that once done is okay to commit to that application? If so, presumably over time eventually all apps will have this already. Some clarification/expansion needed here?

README.md Outdated

`govuk_publishing_components` expects the following components structure in your application:

* a Partial View in `app/views/components` eg `_component-name.html.erb` - The template logic and markup, also defines the arguments expected

This comment has been minimized.

@andysellick

andysellick Jul 20, 2017
Contributor

Having an example file name here is helpful - can example file names also be added for the following items as well?

README.md Outdated
* Documentation in `/app/views/components/docs` - a `.yml` per component, describing the component and containing fixture data.
* a SCSS module in `app/assets/stylesheets/components` - The styling of the component
* a Javascript module in `app/assets/javascripts/components` - JS behaviour of the component
* a unit test in `/test/components` - testing the component renders correctly based on parameters passed in

This comment has been minimized.

@vanitabarrett

vanitabarrett Jul 20, 2017
Contributor

Is it worth pointing people towards an example component somewhere, so they can see exactly what you mean by the above points? Things like 'defines the arguments expected' - it may not be immediately obvious to everyone exactly how this is done?

This comment has been minimized.

@vanitabarrett

vanitabarrett Jul 20, 2017
Contributor

Also, even though we're not giving people a comprehensive guide on how to write a component here, it might still be worth linking to our component principles?

This comment has been minimized.

@fofr

fofr Jul 20, 2017
Author Contributor

Yes, definitely. Holding off doing this until we have one to link to.

@@ -16,13 +15,63 @@ And then execute:
$ bundle
```

This comment has been minimized.

@andysellick

andysellick Jul 20, 2017
Contributor

Where should these commands be executed?

This comment has been minimized.

@fofr

fofr Jul 20, 2017
Author Contributor

These are the standard Rails "How to install gem" instructions.

README.md Outdated

### Configuration

You can configure the gem with a config block in an initializer:

This comment has been minimized.

@andysellick

andysellick Jul 20, 2017
Contributor

Remove 'you can'.

fofr added 4 commits Jul 19, 2017
* Allow use of standard Jenkinsfile by passing `assets:clean` and
`assets:precompile` through to dummy application.
@fofr fofr force-pushed the living-style-guide branch from 32895dd to eaca410 Jul 20, 2017
@fofr
Copy link
Contributor Author

@fofr fofr commented Jul 20, 2017

README updated in b929dd2

@fofr fofr requested review from binaryberry and alecgibson Jul 20, 2017
#### Example documentation

```yaml
name: Withdrawal notice

This comment has been minimized.

@alecgibson

alecgibson Jul 20, 2017

(Just a thought, but it might be nice with all of these examples to demonstrate a single component? Then you can see the process all the way through for one thing. Above for your <%= render example, you use descriptions; for the path examples you use a print link; here you're using a withdrawal notice)

This comment has been minimized.

@fofr

fofr Jul 20, 2017
Author Contributor

Good point. When we've written some I'll link directly to the files in other apps.

The `.yml` file must have:
* a name
* a description
* a default fixture (this can be `{}` if the component takes no parameters)

This comment has been minimized.

@alecgibson

alecgibson Jul 20, 2017

It might be nice to expand on what a "default" fixture is? And what a fixture that isn't default is?

This comment has been minimized.

@fofr

fofr Jul 20, 2017
Author Contributor

I agree. My intention is to fix this, so a component without any fixtures does not take parameters. And an "empty parameter" option can be easily illustrated. Was going to do this as a separate PR.

```ruby
# config/initializers/govuk_publishing_components.rb
GovukPublishingComponents.configure do |c|
c.component_guide_title = "My component guide"

This comment has been minimized.

@alecgibson

alecgibson Jul 20, 2017

What's the use case for being able to configure this?

This comment has been minimized.

@fofr

fofr Jul 20, 2017
Author Contributor

Unique titles for component guides in different apps.

This comment has been minimized.

@alecgibson

alecgibson Jul 21, 2017

I understand what it does, but why would we want to do this?

This comment has been minimized.

@fofr

fofr Jul 21, 2017
Author Contributor

Because each guide will be different. Each guide will have different components.

def fixture
@component_doc = ComponentDoc.get(params[:component])
@component_fixture = @component_doc.fixtures.find { |f| f.id == params[:fixture] }
@guide_breadcrumbs = [

This comment has been minimized.

@alecgibson

alecgibson Jul 20, 2017

It might be nice to break these breadcrumbs out into their own little method.

This comment has been minimized.

@fofr

fofr Jul 21, 2017
Author Contributor

Added in 84537ad

@@ -0,0 +1,34 @@
module GovukPublishingComponents
ComponentDoc = Struct.new(:id, :name, :description, :body, :fixtures) do

This comment has been minimized.

@alecgibson

alecgibson Jul 20, 2017

This class has a lot of collection-level functionality, which feels like it should belong in another separate singleton class that has the ability to memoize things properly, and deliver things from hashes rather than performing find operations.

This comment has been minimized.

@fofr

fofr Jul 20, 2017
Author Contributor

This logic has largely been ported from https://github.com/alphagov/govuk-component-guide/

https://github.com/alphagov/govuk-component-guide/blob/master/app/models/component.rb

I've avoided significant refactoring to keep the two guides similar for now.

fixtures.slice(1..-1)
end

def self.fetch_component_doc

This comment has been minimized.

@alecgibson

alecgibson Jul 20, 2017

fetch_component_docs (plural)?

This comment has been minimized.

@fofr

fofr Jul 21, 2017
Author Contributor

Fixed in d723a47

end

def pretty_data
JSON.pretty_generate(data).gsub('\\n', "\n ").gsub(/"(\w*)":/, '\1:').strip

This comment has been minimized.

@alecgibson

alecgibson Jul 20, 2017

When performing this sort of gsub wizardry, it's often nice to break it out into a one-line mutating method with a nice name so that people can understand what's going on, eg:

def pretty_data
  prettified = JSON.pretty_generate(data)
  prepend_new_lines_with_whitespace!(prettified)
  unquote_properties!(prettified)
  prettified
end

On that note, just why are we doing these things? To make a Ruby-looking hash? In that case maybe we should be using a gem? Or at least rename this method to something like data_as_pretty_printed_ruby_hash?

This comment has been minimized.

@fofr

fofr Jul 20, 2017
Author Contributor

Another ported model from here:
https://github.com/alphagov/govuk-component-guide/blob/master/app/models/fixture.rb#L7

It creates a hash that can be put into a <pre> block with correct formatting. I agree the naming should be better.

end

def fixture
fixtures.first

This comment has been minimized.

@alecgibson

alecgibson Jul 20, 2017

This doesn't necessarily grab the "default" fixture - is that what we want?

This comment has been minimized.

@fofr

fofr Jul 21, 2017
Author Contributor

This relies on convention, you’re right. I think this is ok for now. The convention of putting default first in the fixtures hasn't been an issue with static components.

The alternative would be to lookup via the default ID – if this was left out what should the guide page do? At the moment it'd fallback to the next fixture.

I think when there's a component generator this convention will be natural too.


<div class="component-show">
<div class="component-doc">
<h3>How to call this component</h3>

This comment has been minimized.

@alecgibson

alecgibson Jul 20, 2017

(This view and the fixture view share a bit of code in common. I don't know if it's possible to commonise through a partial?)

fofr added 2 commits Jul 21, 2017
* DRYup breadcrumbs in component guide
* There are many documents and components
Copy link
Contributor

@binaryberry binaryberry left a comment

Nothing to add to existing comments - fine by me once they have been addressed

@fofr
Copy link
Contributor Author

@fofr fofr commented Jul 21, 2017

Plan regarding logic and review comments is to:

@fofr fofr merged commit fb044bd into master Jul 21, 2017
1 check passed
1 check passed
continuous-integration/jenkins/branch This commit looks good
Details
@fofr fofr deleted the living-style-guide branch Jul 21, 2017
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

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