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

Improve HTML attribute merging #256

Merged
merged 5 commits into from
Apr 13, 2021
Merged

Conversation

peteryates
Copy link
Member

@peteryates peteryates commented Apr 3, 2021

Now the formbuilder is using deep_merge internally we run into a problem with some HTML attributes, namely those that represent lists of values as strings. The most common of these are class, aria-describedby aria-labelledby, but there are probably others too - these all contain a space-separated list of values.

The problem is that when we try to merge or deep_merge strings, one gets overwritten:

{class: "red stripes"}.deep_merge({class: "blue spots"})
=> {:class=>"blue spots"}

Instead, if we use arrays:

{class: %w(red stripes) }.deeper_merge({ class: %w(blue spots) })
=> {:class=>["red", "stripes", "blue", "spots"]}

Perfect.

Telling everyone to use arrays is probably a bit heavy-handed so instead there's a new Traits::HTMLAttributes::Attributes class that has the responsibility of doing the conversion to arrays, the merging, and the conversion back to strings. We need to convert back to strings on the way out to get Rails 6.0.3 to pass, Rails 6.1.0+ will happily deal with values in either format.

@peteryates peteryates added the enhancement New feature or request label Apr 3, 2021
peteryates added a commit that referenced this pull request Apr 4, 2021
Rails 6.0.3 doesn't appear to like receiving HTML attribute lists (like
classes) as arrays. This has been improved from Rails 6.1.0 but to
maintain backward-compatibility we're joining the arrays.

This is taken care of by the Attributes class.

Refs #256
peteryates added a commit that referenced this pull request Apr 4, 2021
Rails 6.0.3 doesn't appear to like receiving HTML attribute lists (like
classes) as arrays. This has been improved from Rails 6.1.0 but to
maintain backward-compatibility we're joining the arrays.

This is taken care of by the Attributes class.

Refs #256
@peteryates peteryates force-pushed the improve-html-attribute-merging branch from 79b8f26 to e4aaeb8 Compare April 4, 2021 16:06
peteryates added a commit that referenced this pull request Apr 10, 2021
Rails 6.0.3 doesn't appear to like receiving HTML attribute lists (like
classes) as arrays. This has been improved from Rails 6.1.0 but to
maintain backward-compatibility we're joining the arrays.

This is taken care of by the Attributes class.

Refs #256
@peteryates peteryates force-pushed the improve-html-attribute-merging branch from e4aaeb8 to 80141f6 Compare April 10, 2021 14:50
@peteryates peteryates marked this pull request as ready for review April 10, 2021 14:50
peteryates added a commit that referenced this pull request Apr 11, 2021
Rails 6.0.3 doesn't appear to like receiving HTML attribute lists (like
classes) as arrays. This has been improved from Rails 6.1.0 but to
maintain backward-compatibility we're joining the arrays.

This is taken care of by the Attributes class.

Refs #256
@peteryates peteryates force-pushed the improve-html-attribute-merging branch from 24a1932 to ac0165f Compare April 11, 2021 08:51
This class, GOVUKDesignSystemFormBuilder::Traits::HTMLAttributes::Parser
has the responsibility of transforming the values of HTML attributes that
are lists represented by strings (e.g., class) into arrays.

It's currently a naïve implementation that pays no attention to the
TARGETS constant which will eventually govern which attributes it
modifies.
Rails 6.0.3 doesn't appear to like receiving HTML attribute lists (like
classes) as arrays. This has been improved from Rails 6.1.0 but to
maintain backward-compatibility we're joining the arrays.

This is taken care of by the Attributes class.

Refs #256
In these cases, where the attribute value is nil or an empty string, no
attribute should be added to the element.
@peteryates peteryates force-pushed the improve-html-attribute-merging branch from e7005d9 to b8c171c Compare April 12, 2021 17:24
@peteryates peteryates merged commit c7fd1c4 into master Apr 13, 2021
@peteryates peteryates deleted the improve-html-attribute-merging branch April 13, 2021 11:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants