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

Add support for changing tagnames. #11

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

michaek
Copy link

@michaek michaek commented Oct 12, 2012

Hi there. I really appreciate a simple approach to rendering a calendar!

My use case required rendering without tables - the idea is that it's more semantic and responsive, though I understand that's debatable. :)

So here's the idea: allow a hash in the options for :html_tags, defaulting to the current tags used by the gem, but allowing us to provide our own tags in our implementation. I've also conditionally disabled some attributes that would be illegal on elements other than TD/TH.

I've provided a test case, and the existing tests pass (not to mention that the output still looks ok in practice when I remove my custom :html_tags hash).

I hope this proves useful!

@clemens
Copy link
Owner

clemens commented Oct 15, 2012

Very interesting! The only part I don't like is where you set scope/colspan based on whether or not it's the classic "table" style.

I've actually thought about another approach a while ago but didn't get around to implementing it. I think it would make a lot of sense if the whole table rendering wasn't done inline with content_tag but by rendering partials. Think of the two popular pagination gems will_paginate and kaminari: Currently, later_dude uses an approach similar to will_paginate but I think switching to kaminari-style would make sense. This way, one could provide different styles of showing the calendar (e.g. :table, :semantic, :list) or even enable a kind of theme support.

What do you think? Do you like it? Would you want to implement that? :)

@michaek
Copy link
Author

michaek commented Oct 15, 2012

As far as I understand, scope and colspan aren't valid HTML except on those table elements - that's why the conditions are there.

I think partials are a good idea, but it's a bigger refactor than I could take on now.

@timfjord
Copy link
Contributor

Hi guys
Let me jump here)
So i think the easiest way is to add partial support(later we can add theming, include some predefined themes)
I will try to implement this in the next week

@michaek
Copy link
Author

michaek commented Nov 15, 2012

I'm in favor of partials, but I don't think it should be considered a replacement for these configuration options.

@timfjord
Copy link
Contributor

@michaek But if we use partials i think make sense move all rendering inside it and get rid of all content_tag commands.
Also we need to add override support so everyone can easily customize view

For example(Rails 3 app) create app/views/calendar/_cell.html.erb and update partial

@timfjord
Copy link
Contributor

@michaek but maybe you are right. So we can leave all rendering stuff and just add partial options support

@clemens
Copy link
Owner

clemens commented Nov 15, 2012

As I've said in my comment: An approach similar to kaminari would be nice – later_dude could then also support theming which is something that sounds like a good idea to me. :)

@michaek
Copy link
Author

michaek commented Nov 15, 2012

If everyone's in favor of replacing the current HTML generation with partials, I'm ok with that, so long as it doesn't result in logic bubbling down into the templates - or too much template fragmentation: if I had to override 20 partials to change from table elements to divs, that would be prohibitive.

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.

3 participants