ministryofjustice / govuk_elements_form_builder

Form builder helper methods to develop GOV.UK elements styled applications in Ruby on Rails
https://govuk-elements-rails-guide.herokuapp.com/
MIT License
6 stars 14 forks source link

Support attributes defined as a string #69

Open lostie opened 7 years ago

lostie commented 7 years ago

Fixes https://github.com/ministryofjustice/govuk_elements_form_builder/issues/68

This fix ensures that any element passed to the method is correctly converted to an array:

irb(main):001:0> Array([])
=> []
irb(main):002:0> Array(1)
=> [1]
irb(main):003:0> Array(nil)
=> []
irb(main):004:0> Array('string')
=> ["string"]
irb(main):005:0> Array(:symbol)
=> [:symbol]
irb(main):006:0> Array(['string', :symbol])
=> ["string", :symbol]
aliuk2012 commented 7 years ago

@robmckinnon @misaka @alan @ministryofjustice/ruby Could you review this PR please?

lostie commented 7 years ago

@SteveMarshall ♻️

robmckinnon commented 7 years ago

Thanks for all the attention the gem's getting.

Agreed there's some duplication in spec expectations. This is intentional to make it easy for developers, including those not experienced in Ruby, to quickly see the output html markup for each spec. At the time many specs were copies of example markup from the form section of the GOV.UK elements guide.

The custom RSpec matcher code makes it harder to quickly determine what the expected html markup looks like. You have to think harder to compare markup expectations to the GOV.UK elements guide.

In PR https://github.com/ministryofjustice/govuk_elements_form_builder/pull/71 I've proposed a different way to remove the duplicated name input html expectations, which retains keeping most of the markup expectation in one place. See: https://github.com/ministryofjustice/govuk_elements_form_builder/pull/71 - What do you think?

robmckinnon commented 7 years ago

@SteveMarshall @slorek @aliuk2012 @lostie I find it hard to quickly visualise the html expectations after the spec refactoring in this pull request.

In an alternate pull request, I've had a go at removing spec duplication while retaining an html expectation that was easy to read and understand.

Can you please review this alternate pull request? It's here: https://github.com/ministryofjustice/govuk_elements_form_builder/pull/71

aliuk2012 commented 7 years ago

I like and agree with the "Provide support to Form Builder to define attributes as a string" commit. However, I have to agree with @robmckinnon about spec refactoring, I'm finding it pretty hard to follow.

I think @robmckinnon PR #71 might be a better option as a spec refactor. @lostie maybe it best to split this PR into two. I'd be happy to approve just the attributes as a string and then we can work on refactoring specs as a separate PR.

aliuk2012 commented 7 years ago

@lostie have you been able to do some work on this PR? As I said previously I'm happy with the majority of the pr except for the refactoring of the tests.

robmckinnon commented 7 years ago

@aliuk2012 Maybe you could merge https://github.com/ministryofjustice/govuk_elements_form_builder/pull/71 first? Then the fix for attributes defined as a string could be made on top of that.