Skip to content
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

Suggestion: Take a widget by default, add specializations for Text #9

Closed
felixSchl opened this issue May 3, 2017 · 3 comments
Closed

Comments

@felixSchl
Copy link
Collaborator

felixSchl commented May 3, 2017

As per title, I think this library would benefit from exposing all convenience functions so that they take a widget as the first parameter, as opposed to text to control the <th>. I've read the sources and your comment on the matter, so I know thought went into this, but have you considered:

-- currently:
text :: Text -> (a -> Text) -> Table site a
text h c = widget h (textToWidget . c)

-- but could be (assumes the "widget" helper was updated in the same way):
text :: WidgetT site IO -> (a -> Text) -> Table site a
text w c = widget w  (textToWidget . c)

-- then adding specializations because easy
text' :: Text -> (a -> Text) -> Table site a
text' = text . widgetToText

Do you think this is a worthwhile approach? It seems like a lot of unnecessary code in the library itself but that could be mitigated by pushing specializations downstream, seeing how easy they are to create, given such a widgetToText combinator.

Edit: I should mention my motiviation for this: I would like to be able to control the class/id set on the <th> as i am building a sortable table.

@andrewthad
Copy link
Owner

andrewthad commented May 3, 2017

That would be a good change to make. When I originally wrote this, there was no IsString instance for WidgetT site IO (). However, this got added to yesod. So, we can now use string literals when a widget is expected.

Additionally, there was no ToWidget instance for text. That meant that converting from Text to Widget required this:

toWigdet (toHtml myText)

Now, it's just this:

toWidget myText

So, at this point in time, many of the advantages provided by Text are no longer really advantages. That's my long-winded way of saying "yes", I would like this change. I'll give you a commit bit for this repo in case your interested in making these changes. (I don't particularly care to have the specialized text' function because it's so seldom needed).

For what it's worth, I do not actively maintain this library anymore. In all of my own projects, I have replaced it with colonnade and yesod-colonnade. What colonnade does is basically takes the columnar-encoding logic from yesod-table and makes it backend-agnostic.

I would encourage you to use colonnade instead, but if you'd rather stick with yesod-table, feel free to implement your suggested changes, and I can cut a new release.

@felixSchl
Copy link
Collaborator Author

I was not aware of colonnade, I am reading the sources right now, it looks amazing. In fact, I think I will be using that instead since I am just starting this project out, so I'll close this issue for now. You won me over when I saw the nested table headers, it looks very well thought out.

@andrewthad
Copy link
Owner

Thanks! For the benefit of future passersby, I've added a deprecation notice to the readme for this package, and I've marked this package as deprecated on hackage. In both place, it is mentioned that colonnade and yesod-colonnade are to be preferred.

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

No branches or pull requests

2 participants