Skip to content

Add new view for the checkboxes. - #2043

Closed
polesye wants to merge 1 commit into
masterfrom
anton/add-checkboxes
Closed

Add new view for the checkboxes.#2043
polesye wants to merge 1 commit into
masterfrom
anton/add-checkboxes

Conversation

@polesye

@polesye polesye commented Dec 26, 2013

Copy link
Copy Markdown
Contributor

View for the checkboxes is added in this PR.

@auraz, @cahrens please review.

See screenshots:

screen shot 2013-12-26 at 13 41 24

screen shot 2013-12-26 at 13 45 13

@ghost ghost assigned cahrens Dec 26, 2013
@auraz

auraz commented Dec 26, 2013

Copy link
Copy Markdown
Contributor

@frrrances please review also.

@auraz

auraz commented Dec 26, 2013

Copy link
Copy Markdown
Contributor

@nedbat It is ok to have Checkbox field type on backend?
We need new field type name, to choose new editor_type for it.
Also, currently in master, type list has hardcoded representation on front-end (Select editor).
I think data stored on backend should have different representations on front-end,
What you think about passing editor-type to XField constructor? For example

some_field = List(help="help",
   scope=Scope.setting,
   editor_type=Checkbox # or Select, or whatever
)

May be you have better idea, how to do that.

@auraz

auraz commented Dec 26, 2013

Copy link
Copy Markdown
Contributor

Otherwise 👍

@cahrens

cahrens commented Dec 27, 2013

Copy link
Copy Markdown

I'll take a look at this later today. But my gut feeling, at least for Booleans, is that we should just consistently use the same editor for all Booleans. We could decide that the editor should be a checkbox instead a True/False select, but I don't see why we should present different interfaces for the same type.

I need to look at how List items that allow multiple selections are currently rendered.

@nedbat

nedbat commented Dec 27, 2013

Copy link
Copy Markdown
Contributor

I'm not sure why we need a new field type just to affect the presentation. I like the idea of a modifier that indicates the editor type.

@cahrens

cahrens commented Dec 27, 2013

Copy link
Copy Markdown

List is not currently mapped to a Select. What is mapped to a Select is anything that has a finite list of values defined (note that there is a different List editor type, currently used for html5_sources, where the user can add an arbitrary number of elements). Our current assumption is that when there are a finite list of values, only 1 can be selected.

It seems like what we need is a distinction between multi-select and single-select when there is a finite list of acceptable values. The editors shown for the 2 cases will be different, but I don't think that the xblock field type should say it wants a "Checkbox" vs. a "Select". @nedbat what do you think of this approach? It would mean adding a Field property that would indicate single vs. multi-select. Or perhaps it could just be a property on the values Object itself?

Currently values is loosely defined. For numbers, it looks something like:

values={"min": 0, "step": .1}

whereas for single-select entries, it looks like:

values=[
{"display_name": "Always", "value": "always"},
{"display_name": "On Reset", "value": "onreset"},
{"display_name": "Never", "value": "never"},
{"display_name": "Per Student", "value": "per_student"}
]

Booleans are currently using a Select because the Boolean field type is using values (see below). However, we could choose to render Booleans differently if we think a checkbox is a better interface than a Select for this particular case. But I would want it to be consistent for all Booleans.

super(Boolean, self).init(help, default, scope, display_name,
values=({'display_name': "True", "value": True},
{'display_name': "False", "value": False}))

UX should be involved in this discussion. @marcotuts worked with me on the original editor design.

@auraz

auraz commented Jan 2, 2014

Copy link
Copy Markdown
Contributor

@cahrens you are right, that we are choosing editor depending on data structure of values field.
I would like to change this approach, by setting editor_type attribute of XField to editor type we will use. This value will be passed to front-end, and proper editor will be rendered. I also think we need more broad field property value than ""Checkbox" vs. a "Select", which looks very specific. What you think?

@polesye

polesye commented Jan 9, 2014

Copy link
Copy Markdown
Contributor Author

@marcotuts, @frrrances Could someone of you review this PR?

@frrrances

Copy link
Copy Markdown
Contributor

@polesye I can't comment on some of the more technical discussion above, but in general, I prefer checkboxes or radio buttons for a single setting to a select drop down with true/false. So the example above of the "Download video" seems better than a drop down with "True" or "False". Also, the sass and html looks good, so from my perspective, this is good to go: 👍

@cahrens

cahrens commented Jan 10, 2014

Copy link
Copy Markdown

We still need a proposal for how the editor type will be specified. I really think this comes down to a question of single-select vs. multi-select when there are multiple options present. @polesye, you don't intend to merge this PR as it currently is, do you?

@frrrances Do you agree that we should be consistent for Booleans (always present them in the same way)? And what do you think the presentation should be for Booleans (single checkbox vs. True/False radio buttons)?

@cahrens

cahrens commented Jan 10, 2014

Copy link
Copy Markdown

I'm also curious what the use case is for this work. I don't see anything using it yet.

@polesye

polesye commented Jan 10, 2014

Copy link
Copy Markdown
Contributor Author

Following 2 tickets wait for this PR BLD-364, BLD-368

@Lyla-Fischer

Copy link
Copy Markdown

So, I had no idea that using checkboxes was such a controversial decision. The download transcript and download video stories could do perfectly fine with a True/False dropdown in terms of getting functionality to users. I would prefer leaving the fight for checkboxes as a method of manipulating values in studio for another day.

@cahrens

cahrens commented Jan 10, 2014

Copy link
Copy Markdown

After talking to @Lyla-Fischer, it seems that this is more about making Download Transcript be a Boolean then introducing a checkbox view. Currently Booleans are rendered with a True/False dropdown. @frrrances agrees that if we change the editor for Booleans to checkbox (which is nice), it should be down for all Booleans.

I'm closing this PR because it doesn't seem to be really addressing BLD-364 or BLD-368.

@cahrens cahrens closed this Jan 10, 2014
@benpatterson
benpatterson deleted the anton/add-checkboxes branch January 21, 2015 13:12
jenkins-ks pushed a commit to nttks/edx-platform that referenced this pull request Jul 25, 2017
shimulch pushed a commit to open-craft/openedx-platform that referenced this pull request Jan 26, 2021
…K-1847

YONK-1847 | Fixed the download CSV for TA user.
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.

6 participants