Add customizations for th, tr, and td tags in Table module#14
Conversation
72d838d to
f5ae733
Compare
6664a4c to
92d8b43
Compare
clarktsiory
left a comment
There was a problem hiding this comment.
Nice ! Since it's always a list, and repeats across functions, it could even be factored out
Could you also update the title, so that we have Table in it ? (the library contains something else than table, it makes it easier to trace)
src/Rudder/Table.elm
Outdated
| mapAttribute = | ||
| Html.Attributes.map ParentMsg |
There was a problem hiding this comment.
this could be extracted outside of the view
There was a problem hiding this comment.
this could even be
mapAttributes =
List.map Html.Attributes.map ParentMsg
src/Rudder/Table.elm
Outdated
| let | ||
| rowTable n = | ||
| tr [] | ||
| tr (List.map (Html.Attributes.map ParentMsg) (rowAttrs n)) |
There was a problem hiding this comment.
this could be
| tr (List.map (Html.Attributes.map ParentMsg) (rowAttrs n)) | |
| tr (mapAttributes (rowAttrs n)) |
src/Rudder/Table.elm
Outdated
| |> List.map | ||
| (\{ renderHtml, entryAttrs } -> | ||
| td | ||
| (List.map (Html.Attributes.map ParentMsg) entryAttrs) |
There was a problem hiding this comment.
| (List.map (Html.Attributes.map ParentMsg) entryAttrs) | |
| (mapAttributes entryAttrs) |
src/Rudder/Table.elm
Outdated
| , colspan 1 | ||
| , onClick (SortColumn (ColumnName name)) | ||
| ] | ||
| ++ List.map (Html.Attributes.map ParentMsg) columnAttrs |
There was a problem hiding this comment.
| ++ List.map (Html.Attributes.map ParentMsg) columnAttrs | |
| ++ mapAttributes columnAttrs |
src/Rudder/Table.elm
Outdated
| @@ -800,7 +805,7 @@ view (Model ({ columns, data, options } as model)) = | |||
| [ tableView ] | |||
|
|
|||
| elems -> | |||
| [ div options.customizations.optionsHeaderAttrs elems | |||
| [ div (options.customizations.optionsHeaderAttrs |> List.map mapAttribute) elems | |||
There was a problem hiding this comment.
all could be |> mapAttributes since they are are lists
92d8b43 to
335db4d
Compare
src/Rudder/Table.elm
Outdated
| , columnAttrs : List (Attribute msg) | ||
| , entryAttrs : List (Attribute msg) |
There was a problem hiding this comment.
So, this changes the API of Column significantly : we will need to provide explicit "customizations" on the column every time we instantiate it.
If we are ready to have them within Column, we may need to provide a "column builder" for convenience, as it would more often be empty...
But this primarily raises some inconsistency in the API, these clearly belong to Customizations.
Moreover, the naming of customizations are based on the HTML element that is being customized, respectively th and td here.
And it's true that they are "cell"/"header"-specific, so, is it not possible to have :
type alias Customizations row msg =
...
, thAttrs : Column row msg -> List (Attribute msg)
, tdAttrs : Column row msg -> List (Attribute msg)?
335db4d to
49faa83
Compare
clarktsiory
left a comment
There was a problem hiding this comment.
So, the title of the PR is being divergent with the change that is made, and it indicates there is something still wrong : rowAttrs should be trAttrs 😅
And the real thing this PR is doing is : adding customizations for tr, td, th
49faa83 to
d05ca9d
Compare
This PR aims to
rowAttrscustomization since it was present in the code but actually unused until now.rowAttrstotrAttrsAttribute (Msg msg)are replaced withAttribute msgso that it is easier for the parent component to create attributes that depend on parent msgstdAttrs) and on columns themselves (thAttrs)