Support attributes defined as a string #69
Conversation
|
@robmckinnon @misaka @alan @ministryofjustice/ruby Could you review this PR please? |
There was a problem hiding this comment.
It would be nice to see more detail in the commit history.
In particular, commits should follow the GOV.UK commit guidance and contain a descriptive message body. This might include a brief explanation of the source of the bug/why we care, and why the bug existed. A good (but perhaps slightly extreme?) example of a great commit message is this, from @dcarley.
| @@ -48,6 +48,19 @@ def size(method, size) | |||
| ] | |||
| end | |||
|
|
|||
| it 'supports attributes defined as a string' do | |||
| output = builder.send method, 'name', class: 'custom-class' | |||
There was a problem hiding this comment.
Is there a reason you've used 'name' rather than :name as in the test directly above? It makes it look like two things are varying in this test when it's actually only the class (if I'm reading it right?).
There was a problem hiding this comment.
The whole point of this PR is to support to pass the attribute as a string (the issue is described here), so that's exactly why I've defined this test.
The attribute :name (symbol) is varying in this test to 'name' (string).
There was a problem hiding this comment.
OK, so I'd suggest removing the , class: 'custom-class' from this test, or having a second test for that behaviour.
| '</label>', | ||
| %'<#{element_for(method)} class="form-control custom-class" #{type_for(method, type)}name="person[name]" id="person_name" />', | ||
| '</div>' | ||
| ] |
There was a problem hiding this comment.
It would be nice if the custom-class here weren't buried in the middle of a sea of markup. Maybe it'd be worth refactoring this out to an expected_form_group that accepts parameters, or something like that?
There was a problem hiding this comment.
(And, to be clear, I'd include refactoring out other uses of this same markup as part of that work, not just this instance.)
lostie
force-pushed
the
fix-arrayify-bug
branch
from
c0849df to
ae37d41
Compare
When trying to define a html tag for an attribute which is specified as a string, e.g.:
builder.text_area 'my_form_field'
The following error occurs:
Failure/Error: output = builder.send method, 'name'
NoMethodError:
undefined method `find' for "name":String
Shared Example Group: "input field" called from ./spec/lib/govuk_elements_form_builder/form_builder_spec.rb:272
# ./lib/govuk_elements_form_builder/form_builder.rb:197:in `form_group_classes'
# ./lib/govuk_elements_form_builder/form_builder.rb:29:in `block (2 levels) in <class:FormBuilder>'
# ./spec/lib/govuk_elements_form_builder/form_builder_spec.rb:39:in `block (3 levels) in <top (required)>'
This is raised here as a ruby String also responds to the method :count and the conversion to array is not done appropriately.
The specs that validate what kind of content is generated for all the supported HTML tag built through the Form Builder have a lot of repeated code. This commit tackles that issue by supporting a custom RSpec matcher that validates the HTML output generated by the Form Builder. This commit does NOT change any behaviour in the way that the Form Build works.
lostie
force-pushed
the
fix-arrayify-bug
branch
from
ae37d41 to
f6837e2
Compare
There was a problem hiding this comment.
This is looking ✨ now, @lostie! Might be worth getting one more set of 👀 on it, though?
|
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 #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: #71 - What do you think? |
|
@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: #71 |
|
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. |
|
@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. |
|
@aliuk2012 Maybe you could merge #71 first? Then the fix for attributes defined as a string could be made on top of that. |
Fixes #68
This fix ensures that any element passed to the method is correctly converted to an array: