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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[UX] Replace the classic way of creating select lists/checkboxes/radio buttons with Options Element. #1005

Closed
klonos opened this Issue Jun 13, 2015 · 64 comments

Comments

@klonos
Copy link
Member

klonos commented Jun 13, 2015

Classic way:

select_list_field-settings_classic

Options Element:

select_list_field-settings_options_element

We should get the approval of the maintainer first though 馃槢


Port of the D7 contrib module by @laryn: https://github.com/backdrop-contrib/options_element
PR by @serundeputy: backdrop/backdrop#972
PR by @herbdool: backdrop/backdrop#2204

@klonos

This comment has been minimized.

Copy link
Member

klonos commented Jun 13, 2015

@quicksketch: I saw a couple of issues in the d.org queue were you mentioned that you'd like to see this in core. Particularly interesting is the Provide a reusable API to convert textarea fields to an options element issue because if that happened, then we could use it in #1006.

@quicksketch

This comment has been minimized.

Copy link
Member

quicksketch commented Jun 13, 2015

Thanks @klonos. I like options element myself and we obviously have the ability to put it in core now. I think options element is an improvement on the UX, but for a long-time Drupal user I can't tell if it would be a welcome change or not. I'd like to hear other people's thoughts on the usefulness/intuitiveness of options element. This would be a great candidate for a usability study if we had the resources to do so.

@klonos

This comment has been minimized.

Copy link
Member

klonos commented Jun 14, 2015

...but for a long-time Drupal user I can't tell if it would be a welcome change or not.

I'm a long-time user and I welcome the change. @docwilmot's opinion coming up too...

@Graham-72

This comment has been minimized.

Copy link

Graham-72 commented Jun 14, 2015

I, too, would welcome the change.

@docwilmot

This comment has been minimized.

Copy link
Contributor

docwilmot commented Jun 14, 2015

I welcome this.

@serundeputy

This comment has been minimized.

Copy link
Member

serundeputy commented Jun 20, 2015

I installed the options_element on the D7 and I did not see the new/promised interface.

  • installed options_element
  • added new content type: structure | content types | Add content type
  • added text select list field
    • same old textbox expecting key|value

Is there configuration required?

@quicksketch

This comment has been minimized.

Copy link
Member

quicksketch commented Jun 20, 2015

It should be there. Make sure you've installed the latest stable (1.12) or used the 7.x-1.x branch. Apparently there's a master branch that's outdated.

@serundeputy

This comment has been minimized.

Copy link
Member

serundeputy commented Jun 20, 2015

yep; i was on master thanks! 馃檱

@serundeputy

This comment has been minimized.

Copy link
Member

serundeputy commented Jun 20, 2015

  • there are no tests
  • the drag and drop crosshairs to rearrange the order of the options are not present
    • I'm assuming something with the javascript.
  • there is no config / there were no variable_gets or variable_sets
    • so maybe no config is needed?

PR: backdrop/backdrop#972

@serundeputy

This comment has been minimized.

Copy link
Member

serundeputy commented Jun 20, 2015

Issue #1005: fixing path to js for tabledrag.js and jquery.cookie.js.
updated PR.

@quicksketch

This comment has been minimized.

Copy link
Member

quicksketch commented Jun 22, 2015

Thanks for the PR @serundeputy! I think this element would make the most sense in options.module itself. Although some modules may wish to also use this element (e.g. Webform), I think it would probably be acceptable to depend upon the options.module for this functionality. Thoughts?

@serundeputy

This comment has been minimized.

Copy link
Member

serundeputy commented Jun 22, 2015

@quicksketch

  • combine options_element.module with options.module
  • make field/options/include/ put options_element.inc into it
  • mv js and css into field/css and field/js or should this be field/options/css ... ?
  • add.png and delete.png to `field/options/{images, assets}?

I don't have preference for where it is or with field/options, but if it is cleaner, better, more efficient to be there I'm glad to move it. Let me know if the above steps seem right; and/or clarify the ones that are questions.

thanks,
~Geoff

@docwilmot

This comment has been minimized.

Copy link
Contributor

docwilmot commented Jun 22, 2015

aside, not quite on topic, but:

add.png and delete.png

I dont think those fit with the current admin theme aesthetic. Can we change them?

@quicksketch

This comment has been minimized.

Copy link
Member

quicksketch commented Jun 23, 2015

make field/options/include/ put options_element.inc into it

Our current pattern per #486 is that we leave .inc files in the root, until we establish an official decision in #558. I'd say let's name it "options.element.inc".

mv js and css into field/css and field/js or should this be field/options/css ... ?

These should be in field/options/css. We actually have an issue (and PR) to move all these submodules outside of the field module directory at #557.

add.png and delete.png to `field/options/{images, assets}?

They should go in field/options/images/* yep. Also I think @docwilmot is right that these don't mesh with our current admin theme, but I think they work fine as generic elements. We should probably add Seven-specific styling in Seven's style.css.

@klonos

This comment has been minimized.

Copy link
Member

klonos commented Mar 1, 2016

I would like to "resurrect" this ...and target for 1.4.0 if possible?

@serundeputy if you're not busy with Real Life or anything more important for Backdrop core, if you get the chance to update the PR with the suggestions made, I'll make some time to test and review.

@serundeputy

This comment has been minimized.

Copy link
Member

serundeputy commented Mar 1, 2016

wait; there are things more important than Backdrop? ... :bowtie:

@klonos

This comment has been minimized.

Copy link
Member

klonos commented Mar 1, 2016

馃槃

or anything more important for Backdrop core

I meant any other, more important issues in the Backdrop queue you might be working on.

@laryn

This comment has been minimized.

Copy link
Contributor

laryn commented Sep 26, 2016

I ported options_element in support of @jackaponte's webform_civicrm work (which lists options_element as a dependency) but just came across this thread.

Options_element is not in core yet, is it?

@docwilmot

This comment has been minimized.

Copy link
Contributor

docwilmot commented Sep 26, 2016

Isn't yet. Feel free to continue where @serundeputy left off. :)

@herbdool

This comment has been minimized.

Copy link

herbdool commented May 21, 2018

Reviving this with PR. The one thing I haven't addressed was @docwilmot mentioning that the add/remove images don't match the aesthetic. Could that be a follow-up?

Oh, and no tests. It being mostly JS I think we can't easily test most of that. Perhaps test to look for HTML that should be there?

(Another follow-up: update webform_civicrm to rely on this).

@laryn

This comment has been minimized.

Copy link
Contributor

laryn commented May 21, 2018

Took it for a quick spin and it looks good to me from a UI/usage perspective (noting that there may be a follow-up adjustment to the add/remove images).

Is there a way to disable the contrib options_element by default, too?

@docwilmot

This comment has been minimized.

Copy link
Contributor

docwilmot commented May 22, 2018

Nice work @herbdool

  • on creating a new field if you click manual entry the fieldset for Options settings remains, but then check/unchecking the 'Custom keys' checkbox has no effect. Maybe also hide the whole Options settings fieldset with JS if manual clicked?
  • do we need the Option settings at all? would it simplify the UI (and JS) to just have key and value all the time, and simply tell the user "The value is optional: if a line contains only a key, it will also be used as the value."?
  • the field add/edit screen is normally divided into sections for the particular content type being created/edited, and a section for global settings. BUT with this PR the "Default value" is now moved to the global section. Is that intentional? We cant do per-content-type default values with this? Or is it global values that are gone?
  • simple niggle, when adding key/values, hitting tab moves from key>value>+>X>drag handle>checkbox>key. Can we make it only tab to key>value>nextkey>nextvalue?
  • Not sure I like the "No default" link. I hadnt noticed the checkbox column for defaults and so was clicking that link wondering what its supposed to do. Would a "select/unselect all" checkbox in the tableheader be more intuitive?
  • can a user actually have more than one default value in an option element? If not maybe the JS should enforce this.
  • would also move "Add item" to where "no default" is, if we could.

Only UX review, havent checked code yet.

@herbdool

This comment has been minimized.

Copy link

herbdool commented May 22, 2018

@laryn maybe we'd have to first make an update to the contrib module to set a dependency of system less than a X version.

@herbdool

This comment has been minimized.

Copy link

herbdool commented Oct 1, 2018

@quicksketch I don't think I can completely avoid form alters. I still have list_form_field_ui_field_edit_form_alter so that the options "default widget" can be used instead of the field UI's default widget. I considered using a hook but since it needs to check the field instance to set variables in the field, I don't know if the hook can help.

@jenlampton

This comment has been minimized.

Copy link
Member

jenlampton commented Oct 1, 2018

There's no way that logic for selecting a default widget can go directly into field_ui_field_edit_form()?

@herbdool

This comment has been minimized.

Copy link

herbdool commented Oct 1, 2018

@jenlampton that might not be ideal since it would mean field_ui would then depend on the list and options modules and vice versa.

@jenlampton

This comment has been minimized.

Copy link
Member

jenlampton commented Oct 1, 2018

@herbdool

This comment has been minimized.

Copy link

herbdool commented Oct 1, 2018

I suppose but is that actually better than a form alter? At least with a form alter the logic stays in the module. I noticed there's already an alter for widgets in the list module already.

@herbdool

This comment has been minimized.

Copy link

herbdool commented Oct 2, 2018

This is ready for user testing again.

@jenlampton

This comment has been minimized.

Copy link
Member

jenlampton commented Oct 11, 2018

Wow, I love, love LOVE this improvement! I just tried the sandbox, and its implementation is even better than options element was for webform :) Nice work everyone!

A few thoughts:

  1. Can we change the header column text 'Value' to 'Label' to be consistent with the previous terminology?

value-label

  1. We still have all the help text for the key|label format. Can we have that hidden unless the "Manual entry" toggle has been checked?

manual-entry

  1. Can we move the help text for Allowed HTML tags in labels into a new last row of the table? (or maybe duplicate that text, and hide the lower version unless the "Manual entry" toggle has been checked)

move-help-text

@herbdool

This comment has been minimized.

Copy link

herbdool commented Oct 12, 2018

@jenlampton

  1. I've changed value to label.
  2. It's been moved and hidden along with the manual entry textarea.
  3. I was having more trouble with this one. It has to be visible for regardless of the toggle so that makes it harder.
@herbdool

This comment has been minimized.

Copy link

herbdool commented Oct 12, 2018

@jenlampton 3) is now moved to just below the Add item link. I think it works better there otherwise it breaks up the UI for "Add item" and "No default".

@jenlampton

This comment has been minimized.

Copy link
Member

jenlampton commented Oct 12, 2018

Yes, I LOVE it!

@klonos

This comment has been minimized.

Copy link
Member

klonos commented Oct 19, 2018

I really love this too! ...shamelessly plugging #2304 which would be equally cool IMO.

@jenlampton

This comment has been minimized.

Copy link
Member

jenlampton commented Dec 13, 2018

Ui looks fantastic, all questions on my code review have been resolved. RTBC from me!

@klonos klonos pinned this issue Dec 16, 2018

@docwilmot docwilmot unpinned this issue Dec 17, 2018

backdrop-ci referenced this issue in backdrop/backdrop Dec 27, 2018

@quicksketch

This comment has been minimized.

Copy link
Member

quicksketch commented Dec 27, 2018

I gave this one more code review and it looks great! I've merged this into 1.x for 1.12.0. Thanks @herbdool, @docwilmot, @klonos, @jenlampton, @serundeputy, and @laryn!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment