Skip to content

Conversation

etareduction
Copy link

URL's that point to images are now detected by MIME type using HEAD request, and then displayed in the table cell.

image

@jonatanklosko
Copy link
Member

Hey!

Unfortunately, I don't think we should be doing a synchronous request before rendering the table, and I would prefer to avoid extra dependencies unless strictly necessary. Also, the user may still prefer to render those as actual URLs.

If we are to support it, I think it should be explicit and opt-in. It can be an option like this:

Kino.DataTable.new(..., types: %{key1: "image"})

And type must match one of the supported ones.

@etareduction
Copy link
Author

@jonatanklosko

Hello. I thought about this problem for a while, and got some ideas like what you're saying but didn't want to change the API in any significant way like adding new configuration options without consulting with someone.

What you've shown seems alright to me, i can try re-implementing it this way.


And type must match one of the supported ones.

Also i have updated the types list doc already in this PR.

@jonatanklosko
Copy link
Member

Sounds good!

Also i have updated the types list doc already in this PR.

Oh yes, what I meant is that we would need to check the :types option to make sure it valid types are provided.

@etareduction
Copy link
Author

@jonatanklosko
Sorry for the delay, was busy at my day job last week. Does this last commit look alright to you?

@jonatanklosko
Copy link
Member

@etareduction looks good to me! It would be good to add a test for this as well. It can be similar to this:

test "respects :keys order" do
kino = Kino.DataTable.new(@people_entries, keys: [:name, :id])
data = connect(kino)
assert %{
content: %{
columns: [
%{key: "0", label: ":name"},
%{key: "1", label: ":id"}
]
}
} = data
end

We can pass the :types option and assert that the columns: [...] sent to the client have the expected type field.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants