Opened 7 years ago

Closed 7 years ago

#28712 closed New feature (wontfix)

Add ability to apply separate attributes to ChoiceWidget options

Reported by: Stephen Swatman Owned by: Stephen Swatman
Component: Forms Version: dev
Severity: Normal Keywords: ChoiceWidget
Cc: Tim Martin Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

For ChoiceWidget objects the list of options is created in the create_option method. Currently, options created by this method can either inherit attributes from the parent widget or have no attributes at all but there is no way to separately assign attributes to options as far as I am aware. I would like to suggest introducing the ability to specify separate option attributes in the ChoiceWidget constructor which are applied only to the options in that widget.

Even more powerful would be the ability to pass callables as attribute values which could dynamically compute attributes based on option attributes such as names and values.

Change History (14)

comment:1 by Tim Graham, 7 years ago

Easy pickings: unset

Could you describe your use case in more detail? Is there a reason that overriding ChoiceWidget.create_option() wouldn't work?

in reply to:  1 comment:2 by Stephen Swatman, 7 years ago

Replying to Tim Graham:

Could you describe your use case in more detail? Is there a reason that overriding ChoiceWidget.create_option() wouldn't work?

Web developers may want to individually style each option in a ChoiceWidget, be it by adding CSS classes or style attributes. This may be done to add images to such a widget to make it more appealing to the user. Developers may also want to add ARIA properties to the options or add additional information to work with them via JavaScript.

It would absolutely be possible to override the ChoiceWidget class and modifying the create_option() method. I imagine, however, that this is a common enough use case to warrant a simpler method of doing this. Here are some people on StackOverflow having the same issue:

comment:3 by Stephen Swatman, 7 years ago

Has patch: set
Owner: changed from nobody to Stephen Swatman
Status: newassigned

comment:4 by Tim Graham, 7 years ago

As for the case of styling, wouldn't it involve less markup to set a class on the <select> and use a CSS selector (e.g. select.super option {...}) for that? If anything, I'd think we'd want a way to specify a different attribute value for each <option> value -- setting the same key/value on every <option> doesn't seem like something to encourage.

in reply to:  4 comment:5 by Stephen Swatman, 7 years ago

Replying to Tim Graham:

As for the case of styling, wouldn't it involve less markup to set a class on the <select> and use a CSS selector (e.g. select.super option {...}) for that? If anything, I'd think we'd want a way to specify a different attribute value for each <option> value -- setting the same key/value on every <option> doesn't seem like something to encourage.

You're right that the implementation in the PR was not very powerful. I have now expanded the PR to allow for callable attribute values. When a callable is passed as an attribute value it is separately evaluated for each option and the returned value is used. This would allow developers a lot of control over the attributes of each option.

Last edited 7 years ago by Stephen Swatman (previous) (diff)

comment:6 by Tim Graham, 7 years ago

I still don't find the example in the documentation particularly compelling. Instead of adding CSS classes for that case, I would use CSS [value=x] selectors.

in reply to:  6 comment:7 by Stephen Swatman, 7 years ago

Replying to Tim Graham:

I still don't find the example in the documentation particularly compelling. Instead of adding CSS classes for that case, I would use CSS [value=x] selectors.

The particular case that inspired me to create this issue is that I had a model and I wanted to create a choice widget for that model. I wanted each option to be styled such that it had a background image served from a different server such that I needed to construct an attribute of the form style="background-image: url(https://somewhere.com/images/[id].png)" where the ID was the object's ID. The problem was solved by extending the Select class and overriding the create_option() method. However, I feel it would have been more easily solved had the pull request I created been in place.

Still, I absolutely understand if you think this is too niche. Thus, if I haven't convinced you please feel free to close the issue!

Last edited 7 years ago by Stephen Swatman (previous) (diff)

comment:8 by Tim Graham, 7 years ago

Patch needs improvement: set
Triage Stage: UnreviewedAccepted

I think the functionality could be useful but it would be nice to document a more useful use case. Also, the patch documents the new functionality as Select.opt_attrs but the implementation is on ChoiceWidget. Other classes such as RadioSelect inherit from ChoiceWidget, not from Select.

comment:9 by Stephen Swatman, 7 years ago

Patch needs improvement: unset

I have made an attempt to address your concerns: a more compelling example is given and I have placed it in a more sensible section to cover the entire ChoiceWidget family.

Jenkins is indicating non-passing tests but logs and the fact that only minor documentation changes have been made and that that the docs test still passes lead me to believe this is not an issue with the pull request.

comment:10 by Tim Martin, 7 years ago

Cc: Tim Martin added
Patch needs improvement: set

I added a couple of minor comments on the documentation in the pull request.

in reply to:  10 comment:11 by Stephen Swatman, 7 years ago

Patch needs improvement: unset

Replying to Tim Martin:

I added a couple of minor comments on the documentation in the pull request.

Thank you! I have amended the mistakes you pointed out.

comment:12 by Bjorn Kristinsson, 7 years ago

I ran into a case the other day where this would have been extremely useful. I needed to show a select with all options visible, but depending on the user's permissions some of the options needed to be disabled. This can be achieved by setting the disabled attribute, but since there was no way to do that per option, I had to roll my own.

Looks like this would solve that, and if you're still looking for an example to use in the docs, maybe you could show how to disable options?

comment:13 by Carlton Gibson, 7 years ago

Patch needs improvement: set

I've left a review on the patch.

My main concern is the documentation: we need to properly document ChoiceWidget so that this new feature would make sense.

Beyond that (also on the PR) I think we should be advocating the subclass and override create_option approach. I know people always want an extra keyword arg, but such an approach is both lightweight and maintainable (both for Django and user code).

That approach is under-documented currently. I would advocate closing this as wontfix and creating a new ticket to properly document ChoiceWidget.

comment:14 by Stephen Swatman, 7 years ago

Resolution: wontfix
Status: assignedclosed
Note: See TracTickets for help on using tickets.
Back to Top