Allow ranges to be passed for :list props #361
Conversation
|
updated the CI matrix as well here to only support 1.11 and up |
|
LGTM. @msaraiva If I well understood, we should merge this to the https://github.com/surface-ui/surface/tree/surface-next branch? |
|
@msaraiva rebased this against |
lib/surface/type_handler/list.ex
Outdated
| case unquote(expr) do | ||
| value when is_list(value) -> | ||
| value when is_list(value) or is_struct(value, Range) -> | ||
| value |
There was a problem hiding this comment.
@davydog187 maybe we should call Enum.to_list/1 to make sure the value will be a list (unless this is already done somewhere else?). After all, the value is defined as :list so the user might have code depending on the value being a list, e.g. List.first/1 and friends. This is similar to what we do when accepting keyword lists in props defined as :map, we accept it as syntactic sugar but always convert it to map. WDYT?
There was a problem hiding this comment.
Seems reasonable. Effectively you're suggesting doing an implicit cast so that when it crosses the component boundary, it's in the type expected. I buy that
There was a problem hiding this comment.
Exactly! If the user actually needs a range, then it's better to explicitly define the prop as :range.
There was a problem hiding this comment.
Ok, updated the code, feels like it should be tested though! Is compiler_test.exs the best place to test this?
Nevermind, this gets caught by my current thests 👍🏼
There was a problem hiding this comment.
Done. Now the tests make more sense too. Good call @msaraiva
|
@davydog187 thanks! ❤️ |
3 participants
Similar to #355, allows ranges to be passed for
:listvalues. This is particularly useful when you have a fixed number of components that would be generated from within the prop. I found the need for this when implementing aRadioGroupcomponent that composesSurface.Components.Form.RadioButtonThere is an argument for allowing any enumerable to be passed as a list, I'm not sure if that's a good idea or not.
Example