-
Notifications
You must be signed in to change notification settings - Fork 21
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
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
peteryates
commented
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
force-pushed
the
improve-html-attribute-merging
branch
from
April 4, 2021 16:06
79b8f26
to
e4aaeb8
Compare
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
force-pushed
the
improve-html-attribute-merging
branch
from
April 10, 2021 14:50
e4aaeb8
to
80141f6
Compare
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
force-pushed
the
improve-html-attribute-merging
branch
from
April 11, 2021 08:51
24a1932
to
ac0165f
Compare
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
force-pushed
the
improve-html-attribute-merging
branch
from
April 12, 2021 17:24
e7005d9
to
b8c171c
Compare
cpjmcquillan
approved these changes
Apr 13, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
ordeep_merge
strings, one gets overwritten:Instead, if we use arrays:
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.