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

Feature: Widget's css_class should default to field name #97

Closed
rbu opened this issue Jun 22, 2012 · 18 comments
Closed

Feature: Widget's css_class should default to field name #97

rbu opened this issue Jun 22, 2012 · 18 comments

Comments

@rbu
Copy link
Contributor

rbu commented Jun 22, 2012

Often times, we want to apply styles to certain form elements that can easily done purely in CSS. It feels like overkill to create a new widget for every schema node just to set the css class.

It would be a lot more helpful if the css_class of deform's Widget would default to a daherized/sanitized version of field.name with "deform-" prepended.

@rbu
Copy link
Contributor Author

rbu commented Aug 24, 2012

I'm happy to provide a patch, but I would appreciate general consent with the idea first so I don't provide waste.

rbu added a commit to rbu/deform that referenced this issue Aug 29, 2012
@rbu
Copy link
Contributor Author

rbu commented Aug 29, 2012

see referenced patch for a proposed implementation

@kiorky
Copy link
Member

kiorky commented Sep 21, 2012

Well, fine for me; rebase your patch !
@mcdonc any objection ?

@mcdonc
Copy link
Member

mcdonc commented Sep 21, 2012

AN issue with this is that forms that have mappings and sequences may have fields with the same name.

@kiorky
Copy link
Member

kiorky commented Sep 21, 2012

For those, we can add a -X to identify them ?
like class="fieldname fieldname-0"

@mcdonc
Copy link
Member

mcdonc commented Sep 21, 2012

I think it might defeat the purpose, because the number would change if more fields were added.

@kiorky
Copy link
Member

kiorky commented Sep 21, 2012

looking to c8df92b ref in #98, i think this is not necessary to have css classes on "li values" because you can easily match the li in css with #97 or even a49904f with "ul.fieldname li"

@rbu
Copy link
Contributor Author

rbu commented Oct 24, 2012

Rebasing this was a mess, I have done so in this branch: https://github.com/rbu/deform/tree/issue-97-98

Regarding uniqueness. These auto-generated css_classes are not unique, but I did not indend them to be. They are intended as a stable way to address a certain field's widget and style it (e.g. 'deform-field-username'). If you have multiple such fields in a sequence and want to address them individually, one could use :nth-child (plus a polyfill if needed).

rbu added a commit to rbu/deform that referenced this issue Oct 24, 2012
rbu added a commit to rbu/deform that referenced this issue Oct 24, 2012
@rbu
Copy link
Contributor Author

rbu commented Nov 22, 2012

ping

@mcdonc
Copy link
Member

mcdonc commented Jan 10, 2013

Ugh, sorry for the tremendously late response on this. This code looks great, but I'm a bit cramped for time to really evaluate it to see if it breaks anything. I just wanted to let you know it's not forgotten about.

@rbu
Copy link
Contributor Author

rbu commented Jan 15, 2013

Thanks for getting back. We've been holding back on relying on this for production since we don't want to maintain a fork, but it would be very helpful for our form styling to get this in and start relying on it.

@dwt
Copy link
Contributor

dwt commented Jan 28, 2013

Ping again. This would be really helpful for us in production too, what about the merge status?

@dwt
Copy link
Contributor

dwt commented Feb 1, 2013

ping... We're about to package this patch for ourselves as it would be so usefull - could you please at least give a word what the status of this is?

@dwt
Copy link
Contributor

dwt commented Feb 4, 2013

I recognize that I may be a bit anxious, but could you please reply here and give a status update?

@dwt
Copy link
Contributor

dwt commented Feb 11, 2013

Well, here we go again... ping? Any news from the maintainers?

@rbu
Copy link
Contributor Author

rbu commented Feb 15, 2013

@kiorky @mcdonc This bug report seems to be more noise than discussion about the feature. Is there anything we can do to get this feature merged soon?

On a general note, I understand if you oppose a certain feature in your library (even if I may end up debating this with you), but getting positive feedback and no merge in 6 months is rather discouraging (for providing pull requests for other bugs that I opened).

@mcdonc
Copy link
Member

mcdonc commented Feb 15, 2013

Ugh. Sorry. I don't oppose the feature at all. My status on it is still the same: I'm still trying to figure out how to get the time to find out if it doesn't break anything. It's piled below a bunch of other things that are also in the same status. I'm a terrible maintainer, but it is what it is at this point, just trying to tread water. Sorry.

@cguardia
Copy link
Member

This is being addressed on issue #131. Closing this to reduce noise.

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

5 participants