Conversation
Support classes set using the `class` attribute
…ve-old-node-versions Update CI test matrix
There was a problem hiding this comment.
Pull request overview
This PR bumps the version from 2.2.0 to 2.2.1 and adds support for testing HTML classes set using the class attribute (in addition to the existing support for className property). The implementation adds a validation system that detects when both class and className are used on the same element, which would result in undefined behavior.
Changes:
- Added validation to detect and report errors when both
classattribute andclassNameproperty are used on the same HTML node - Enhanced the classnames query function to support both
classattribute andclassNameproperty - Added tests to verify class matching works with
Attr.attribute,Attr.property, and validates against mixing both - Updated CI to test against newer Node.js versions (16, 18, 20, 22, 24) and upgraded elm-tooling-action to v1.7
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| elm.json | Version bump from 2.2.0 to 2.2.1 |
| CHANGELOG.md | Added release notes for version 2.2.1 documenting the new class attribute support |
| .github/workflows/ci.yml | Updated Node.js test matrix to versions 16-24 and elm-tooling-action to v1.7 |
| src/Test/Html/Internal/ElmHtml/InternalTypes.elm | Added Validation type system and validation logic to detect mixed use of class/className |
| src/Test/Html/Internal/Inert.elm | Updated error handling to distinguish between decode errors and validation errors |
| src/Test/Html/Internal/ElmHtml/Query.elm | Enhanced classnames function to support both class attribute and className property |
| src/Test/Html/Query/Internal.elm | Added ValidationErrors variant to Query type and propagated validation errors through the query system |
| src/Test/Html/Query.elm | Updated fromHtml to handle new error types from Inert module |
| src/MicroListExtra.elm | Added unique function to deduplicate validation errors |
| tests/src/Test/Html/QueryTests.elm | Added tests for class matching with Attr.attribute, Attr.property, and mixed usage validation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| uniqueHelp identity [] list [] | ||
|
|
||
|
|
||
| uniqueHelp : (a -> b) -> List b -> List a -> List a -> List a | ||
| uniqueHelp f existing remaining accumulator = | ||
| case remaining of | ||
| [] -> | ||
| List.reverse accumulator | ||
|
|
||
| first :: rest -> | ||
| let | ||
| computedFirst = | ||
| f first | ||
| in | ||
| if List.member computedFirst existing then | ||
| uniqueHelp f existing rest accumulator | ||
|
|
||
| else | ||
| uniqueHelp f (computedFirst :: existing) rest (first :: accumulator) |
There was a problem hiding this comment.
The uniqueHelp function accepts a transformation function parameter f that's always called with identity. This suggests either the function was designed for future extensibility or was copied from a more generic implementation. Consider simplifying by removing the f parameter and directly using the list elements in the comparison, or if the flexibility is intentional for future use, add a comment explaining this design choice.
| uniqueHelp identity [] list [] | |
| uniqueHelp : (a -> b) -> List b -> List a -> List a -> List a | |
| uniqueHelp f existing remaining accumulator = | |
| case remaining of | |
| [] -> | |
| List.reverse accumulator | |
| first :: rest -> | |
| let | |
| computedFirst = | |
| f first | |
| in | |
| if List.member computedFirst existing then | |
| uniqueHelp f existing rest accumulator | |
| else | |
| uniqueHelp f (computedFirst :: existing) rest (first :: accumulator) | |
| uniqueHelp [] list [] | |
| uniqueHelp : List a -> List a -> List a -> List a | |
| uniqueHelp existing remaining accumulator = | |
| case remaining of | |
| [] -> | |
| List.reverse accumulator | |
| first :: rest -> | |
| if List.member first existing then | |
| uniqueHelp existing rest accumulator | |
| else | |
| uniqueHelp (first :: existing) rest (first :: accumulator) |
| divWithAttribute (Attr.property "className" (Encode.string "hello")) | ||
| |> Query.fromHtml | ||
| |> Query.has [ class "hello" ] | ||
| , test "matches nothing if classes are added both using Attr.attribute and Attr.property" <| |
There was a problem hiding this comment.
The test name "matches nothing if classes are added both using Attr.attribute and Attr.property" could be misleading. It suggests the query succeeds but returns no matches, when in fact it fails with a validation error. Consider a more descriptive name like "fails with validation error when classes are added using both Attr.attribute and Attr.property" to better reflect the actual behavior being tested.
| , test "matches nothing if classes are added both using Attr.attribute and Attr.property" <| | |
| , test "fails with validation error when classes are added using both Attr.attribute and Attr.property" <| |
No description provided.