As mentioned in #675116: field.api.php hook documentation is incomplete we have a hook for field formatter settings that's not getting used because the UI hasn't been figured out.

Following on from #553298: Redesign the 'Manage Display' screen, let's figure it out here. It would be a real shame to lose this feature in D7: lack of formatter settings in CCK was a big gap, and let to the problem of dozens of formatters per field.

CommentFileSizeAuthor
#184 field_ui-display-settings-d7.png32.34 KBandypost
#163 field_ui_formatter_settings-796658-163.demo_.patch.txt24.71 KByched
#163 field_ui_formatter_settings-796658-163.patch19.06 KByched
#148 field_ui_formatter_settings-796658-136-demo.patch.txt30.37 KByched
#148 field_ui_formatter_settings-796658-139.patch24.38 KByched
#136 field_ui_formatter_settings-796658-136.patch30.37 KByched
#139 field_ui_formatter_settings-796658-139.patch24.38 KByched
#133 field_ui_formatter_settings-796658-133.patch30.32 KByched
#129 field_ui_formatter_settings-796658-129.patch28.63 KByched
#128 field_ui_formatter_settings-796658-128.patch28.57 KByched
#127 formatter_settings-127.png22.03 KByched
#127 field_ui_formatter_settings-796658-127.patch27.12 KByched
#114 field_ui_formatter_settings-796658-113.patch26.93 KByched
#114 formatter_settings-113.png22.36 KByched
#109 field_ui_formatter_settings-796658-108.patch19.99 KByched
#109 field_ui_formatter_settings-796658-108-demo.patch.txt25.99 KByched
#104 formatter_settings-104.png12.76 KBandypost
#103 formatter_settings-103.png22.12 KByched
#103 field_ui_formatter_settings-796658-103.patch25.36 KByched
#99 screenshot-changes-formatter-ui.png80.38 KBBojhan
#95 formatter_settings-94.png22.8 KByched
#92 formatter_settings_92-1.png46.13 KByched
#92 formatter_settings_92-2.png45.32 KByched
#91 formatter_settings_91.png7.14 KByched
#87 formatter_settings_87.png109.4 KByched
#90 formatter_settings_90.png6.59 KByched
#82 formatter_settings.png38.54 KByched
#82 field_ui_formatter_settings-796658-82.patch21.67 KByched
#78 field_ui_formatter_settings-796658-78.patch21.05 KByched
#73 fieldsettings.jpg129.05 KByoroy
#67 field_ui_formatter_settings-796658-67.patch21.6 KByched
#67 field_formatter_edit.png21.38 KByched
#64 field_ui_formatter_settings-796658-debug.patch.txt20.92 KByched
#61 field_ui_formatter_settings-796658-61.patch21.55 KByched
#58 formatter_settings_button1.png6.56 KByched
#58 formatter_settings_button2.png7.31 KByched
#58 formatter_settings_button3.png7.26 KByched
#58 formatter_settings.png19.23 KByched
#58 field_ui_formatter_settings-796658-58.patch21.49 KByched
#43 block-configure.png2.66 KByched
#43 module-configure.png1020 bytesyched
#43 formatter-configure-2.png4 KByched
#43 formatter-configure.png4.01 KByched
#40 field_ui_formatter_settings-796658-40.patch21.61 KByched
#40 field_formatter_edit.png21.49 KByched
#38 field_formatter_warning.png21.24 KByched
#37 field_ui_formatter_settings-796658-37.patch21.65 KByched
#33 field_ui_formatter_settings-796658-33.patch21.64 KByched
#31 field_formatter_summary.png19.19 KByched
#29 field_ui_formatter_settings-796658-29.patch21.38 KByched
#26 form1-1.gif18.1 KBandypost
#25 screen_collapsed.png2.27 KBandypost
#25 screen_collapsed2.png3.09 KBandypost
#20 field_ui_formatter_settings-796658-20.patch21.38 KByched
#20 formatter_settings_summary.png22.22 KByched
#20 formatter_settings_edit.png22.5 KByched
#20 formatter_settings_warning.png24.43 KByched
#17 796658-field_ui-settings-d7.patch18.12 KBandypost
#14 796658-field_ui-settings-d7.patch18.18 KBandypost
#9 field_ui_formatter_settings-796658.patch16.64 KByched
#9 formatter_settings_summary.png21.64 KByched
#9 formatter_settings_edit.png22.26 KByched
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

yoroy’s picture

Issue tags: +Usability, +ui-pattern

My simple mind can't tell from code alone what exactly this ui would be for. Can you help visualize this? Is there a similar thing in D6 CCK for instance? Thanks. Tagging…

joachim’s picture

There isn't anything in D6 for this.

All we have to go on is the hook documentation, which was written before implementing any use of the hook.

From the patch at #675116 which removes it:

 /**
- * Formatter settings form.
- *
- * @todo Not implemented yet. The signature below is only prospective, but
- * providing $instance is not enough, since one $instance holds several display
- * settings.
- *
- * @param $formatter
- *   The type of the formatter being configured.
- * @param $settings
- *   The current values of the formatter settings.
- * @param $field
- *   The field structure being configured.
- * @param $instance
- *   The instance structure being configured.
- * @return
- *   The form definition for the formatter settings.
- */
-function hook_field_formatter_settings_form($formatter, $settings, $field, $instance) {
-}

We'd have to ask yched or someone else who's worked on FieldAPI for more of the detail, but I imagine it would allow us to have things like one formatter for images and then a form to choose options, rather than a growing dropdown of image size and link combinations.

yched’s picture

"I imagine it would allow us to have things like one formatter for images and then a form to choose options, rather than a growing dropdown of image size and link combinations."

Exactly. Just like you can chose a widget type, and then refine settings for this widget (e.g number of rows for a textarea, or width for a textfield).

Add to the example for images : in CCK D6, there are 3 formatters per imagecache style (image, image linked to file, image linked to node). Enable Lightbox module, and you additionally get one "lightbox" formatter per imagecache style... endlesss list...
Without a UI for formatter settings, all output variants need to be exposed as separate formatter types. Dreadful for large numbers of combinations (see above), and no-go for numeric "parameters" (length, size... see below).

Other examples :
- text fields (eg body) : size of truncated summary should be a formatter setting, instead of currently relegated to a separate UI in the 'edit node type' form (thus applied to all text fields in the node type, and non relevant for non-node entities, which get a hardcoded default). See #504564: Make summary length behave with fields.
- number fields : output format (number of decimals, chars for thousands and decimal separators) are already formatter settings in D7, but are currently out of reach of the UI.

Reminder : the Field *API* already supports formatter settings. You can specify then when programmatically creating / modifying a field instance with field_[create|update]_instance(). It's "just" the UI that doesn't support them.

yched’s picture

Challenges :

- there's obviously a dependency between the selected formatter type and the content of the corresponding settings form. Raises a UI workflow question :
1- pick the formatter in the dropdown list
2- hit "submit" so the formatter is saved
3- only then, click the 'settings' link so you can adjust the settings for this formatter ?
Ugly and very tedious.

- when visually skimming over the 'Manage display' page, you need to be able to see which field uses which formatter, but also get a glimpse of the settings used for each formatter without having to click a link to open a separate form just to check the values. This calls for a notion of "settings summary", and has screen real-estate implications.

Views and Panels UIs have their own answers for those exact same UI challenges : AHAH subforms (in popups or inlined), summaries. But there's quite some engineering behind them...

joachim’s picture

I don't know enough about what's possible in D7 FormAPI for this -- do we have dependent form elements like in the Views 2 UI?

Could we also get some UI people on the case for ideas?

yched’s picture

"do we have dependent form elements like in the Views 2 UI?"

We do, but this "only" hides / shows parts of a form depending on the state of another form element. That would mean generating a form with all settings for all available formatters. Multiply this by the number of fields listed in the "Manage display" table = damn heavy form...
+ this absolutely does not degrade without JS.

Dependant form elements are not the core mechanisms for Views answers to those UI challenges (as opposed to AJAX popups / panels)

joachim’s picture

yched’s picture

Title: FieldAPI: UI for formatter settings: hook_field_formatter_settings_form » UI for field formatter settings
Component: field system » field_ui.module

Retitle.

Actually giving it a try, turns out this might not be as far away as I initially thought. Will try to post a proof-of-concept patch soon.

yched’s picture

Status: Active » Needs review
Issue tags: +Needs usability review
FileSize
22.26 KB
21.64 KB
16.64 KB

Initial patch. Still needs some visual and layout polish, but seems to work fairly well.

For testing's sake, added 'formatter settings' to
- image field formatters
- text field "Trimmed' and "Summary or trimmed' formatters.

Best place to test is to go to admin/structure/types/manage/article/display/teaser

Attached a couple screenshots too.

Status: Needs review » Needs work
Issue tags: -Needs usability review, -Usability, -ui-pattern

The last submitted patch, field_ui_formatter_settings-796658.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
Issue tags: +Needs usability review, +Usability, +ui-pattern
yched’s picture

forgot to add : this is of course ajax-driven, and degrades nicely to a multistep form.

Status: Needs review » Needs work

The last submitted patch, field_ui_formatter_settings-796658.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
FileSize
18.18 KB

Here is a fixed patch and review.

Patch adds actual trimming to text field - tested and it works!

Also found a bug with 'text_trimmed' - size for bundle (node type) was ignored see 'text_summary_or_trimmed' implementation.
$size = variable_get('teaser_length_' . $instance['bundle'], 600); should be moved out of loop

Status: Needs review » Needs work

The last submitted patch, 796658-field_ui-settings-d7.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review

From usability point of view:

1) there's no indication in UI that settings changed! It takes a lot of time to understand that I need to press "Save" at the bottom to apply settings. I think this needs something like table drag does... Same applies to changing formatter - no visual indication!

2) maybe put settings into next table cell? Some fields could have a lot of settings so screen could confuse people. OTOH settings depands on formatter so they should be a part of "Format" cell

andypost’s picture

Patch for win line-ending posted as follow-up #553298-160: Redesign the 'Manage Display' screen

So this patch without this change

Status: Needs review » Needs work

The last submitted patch, 796658-field_ui-settings-d7.patch, failed testing.

yched’s picture

Status: Needs work » Needs review

windows line endings - hm, strange. All my patches are rolled through dos2unix, and are usually fine. Will double check.

The settings for 'trim length' were added here mainly to have something to chew on with the fields present in a default install. The actual implementation, and cleanup of the existing half-baked 'teaser_length_[bundle]' variables should be done in #504564: Make summary length behave with fields

Yup, a message for "no changes are saved until you hit 'Save'" is probably in order here.

I'm also wondering about putting the settings in a dedicated cell - nested under 'Format' header with a colspan.
At any rate, the 'settings summary / settings form' should be pushed to the right of the 'formatter type' select (not below like in current patch). In the same cell / in a separate cell is mainly a question of alignment :
- either settings appear just to the right of the select in the same cell, meaning they are not vertically aligned with one another, because the selects for different field types have different widths.
- either we push them to separate cells, which makes them vertically aligned, but makes the connection with the 'formatter type' select less obvious, and wastes some screen real estate

As I said above, the layout still needs some care and consideration. Help highly welcome, for that matter :-).

Also, the 'Settings' button that lets you display the settings form is way too intrusive, IMO it should be replaced by a "cogwheel" image - not sure of the best way to achieve that. #647228: Links are needlessly unable to fully participate in D7 AJAX framework features looks highly debated still.

yched’s picture

New patch
- moves settings to their own cell
- adds a warning that changes won't be saved until the form is submitted. (still need to resolve a duplication with tabledrag's own message)

Updated screenshots too.

Status: Needs review » Needs work

The last submitted patch, field_ui_formatter_settings-796658-20.patch, failed testing.

Bojhan’s picture

Status: Needs work » Needs review

Why don't we push these settings off to a separate page? Ie having a Formatter link alongside each field? Either way the interaction if you want to go down this road should be similar to that of content type [edit] interaction.

andypost’s picture

Looking at formatter_settings_warning.png I think that changing cell color is enough and there's no need in message above.

@Bojhan suppose moving settings to separate page is bad because user should additional click to overview settings and another click to change them. Also I think that formatters mostly have seriously less settings then field options.

aspilicious’s picture

The message is needed as not everyone knows what the yellow color indicates.

andypost’s picture

Another idea - make settings collapsed under link like l10_server does.
Button "Settings" could be changed to ajaxed text-link "Change options"

andypost’s picture

FileSize
18.1 KB

Another round...

Suppose Delete could be changed to "reset to defaults"

yched’s picture

Title: UI for field formatter settings » UI for fiEither way the interaction if you want to go down this road should be similar to that of content eld formatter settings

re Bojhan #22
"Why don't we push these settings off to a separate page? "
Because :
- we don't want users to click 10 links and have 10 page reloads to adjust the formatters used for the 10 fields in their content type. That would be a major regression compared to the current UI. Also, you can't have formatter selection in a separate form than the settings selection, because you don't want the actual submission and update of the actual instance definition to happen in separate steps (i.e save with a different formatter, and later save with corresponding adjusted settings, because meanwhile your site displays crap)
We also need a visual overview on one page of which fields use which formatters with which settings.
- there is a dependency between the selected formatter and the options form available. This requires some ajax update / multistep relaod.

"Either way the interaction if you want to go down this road should be similar to that of content type [edit] interaction"
Not sure which interaction you refer to exactly, but if formatter settings never made it to CCK contrib in 2+ years and still don't have a UI so far, it's precisely because the existing UI patterns didn't apply to the constraints here :-)

I'm not sure I understand where the proposal in #26 goes. I don't get the 'delete' button (nor the need for a 'get back to default') and the 'add new option' select ?

As I said above, IMO the main remaining visual adjustment is to turn the intrusive "Edit" button in this screenshot into a cogwheel clickable image.

yched’s picture

Title: UI for fiEither way the interaction if you want to go down this road should be similar to that of content eld formatter settings » UI for field formatter settings

Oops. Obviously some copy/paste went terribly wrong. Setting back title.

yched’s picture

Updated patch just fixes the duplication with tabledrag's own message.
Patch ensures one single instance of the "Changes made in this table will not be saved until the form is submitted." message is displayed, ever (requires a small change to tabledrag.js to handle the message display in an overridable method)

Status: Needs review » Needs work

The last submitted patch, field_ui_formatter_settings-796658-29.patch, failed testing.

yched’s picture

FileSize
19.19 KB

FWIW, here's a screenshot using an image button for the 'edit settings' button.

No patch yet, because there seems to be a bug with FAPI #type = 'image_button', messing with form submission. Digging.

yched’s picture

filed a bug at #809292: FAPI #type image_button broken [edit: forget it, I'm a jerk]

yched’s picture

Status: Needs work » Needs review
FileSize
21.64 KB

Here's a working patch with an 'image_button' (screenshot in #31 above)

Status: Needs review » Needs work

The last submitted patch, field_ui_formatter_settings-796658-33.patch, failed testing.

joachim’s picture

I get a failed at:

1 out of 3 hunks FAILED -- saving rejects to file modules/field_ui/field_ui-display-overview-table.tpl.php.rej

aspilicious’s picture

Status: Needs work » Needs review
yched’s picture

re Bojhan #22 :
Doh, I finally understood "Either way the interaction should be similar to that of content type [edit] interaction" - you mean the [edit] JS-link to display the 'content type machine-name' input on admin/structure/types/manage/article/edit.
(re-reading your sentence, it was fairly obvious, I don't know why I didn't click til now)

Well, it's technically a bit different. The 'machine-name [edit]' link is just a JS show/hide of something that is already in the HTML.
Here we need to actually submit the form to update a sub-part in an AJAX request (or regular page reload if no JS). Doing so with a simple link instead of a button is still debated in #753270: Impossible to trigger form submit with a link and #647228: Links are needlessly unable to fully participate in D7 AJAX framework features, and isn't doable currently.

What do you think of the screenshot in #31 ? An image-button rather than a text link also saves screen real-estate (also taking translations into account), which is more a concern here than for the machine-name interaction.

Attached patch adds 'Edit' as an 'alt' attribute on the button.

Summary of the screenshots :
step 1
step 2
step 3

yched’s picture

FileSize
21.24 KB

Updated shot for the 3rd step

Summary of the screenshots :
step 1
step 2
step 3

tstoeckler’s picture

In step 2 is it intentional that the Image formatter (which is not being edited) shows as a button and not as an image?
I think that is somewhat inconsistent.

yched’s picture

No of course, the screenshot for step 2 needs to be updated as well :-)

Updated patch adds CSS margin above the two 'Save' and 'Cancel' buttons.

Screenshots summary :
step 1
step 2
step 3

Bojhan’s picture

@yched You should use [edit] interaction here, you can't use the clog - that should be used in a way different context and doesn't prone a mental model that one goes into editing mode.

tstoeckler’s picture

yched in #37:

Doing so with a simple link instead of a button is still debated in #753270: Impossible to trigger form submit with a link and #647228: Links are needlessly unable to fully participate in D7 AJAX framework features, and isn't doable currently.

Since we're tied to an image, I guess that cog thingy is the best we can get without those issues. If those land, we should definitely keep consistency with the machine-name [edit] thing.
Or someone has an idea for a better image.

yched’s picture

[edit: crosspost with #42]

@Bojhan : Plz see #37.
[edit] link is currently not possible for multistep forms, and a regular form button is too intrusive / eats too much real-estate. Any other suggestion ? :-) A different icon ?

For that matter, the cogwheel is named 'configure.png', which is precisely what it does here, and has been used by Views UI for this exact kind of stuff for years. In current code D7, the cogwheel is used to access configuration of an 'element' (a block, a module). No hijack here.

So quite to the contrary, the icon itself seems perfectly suited to me, and IMO introducing a different icon would make things more confusing.
I think what's bothering you, rather than the icon itself, is the fact that the way it's presented doesn't let users predict what kind of interaction happens when they click on it (dropdown, AJAX update, navigate to new page...).

Those two informations ("what ?" and "how ?") don't have to and cannot necessarily be carried by the image itself.
In the existing core examples, the cogwheel image only conveys "what", and "how" is conveyed by:
- for block config, a 'dropdown arrow', indicating that a dropdown menu will open
block-configure.png

- for module config, the 'configure' link itself, indicating that this will take you to another page.
module-configure.png

--> proposals :
- a more 'buttony' version :
formatter-configure.png

- something like the 'dropdown arrow' used for blocks - with the 'unfolded' button acting like 'Cancel' ?
formatter-configure-2.png

Bojhan’s picture

@yched I get that Views does it that way, doesn't mean that it fits here. I think using it like this, whatever way you make it look will not make it more usable - and does trigger a different mental model. That [edit] interaction isn't possible, shouldn't be keeping us back - lets just fix it, or hack it in for the time being, rather than choosing an solution that is bad. I am really feeling strongly about this, lets not introduce more confusion when we have a perfectly fitting interaction in core, that it doesn't work from a technical perspective should be perfectly fixable.

amc’s picture

The arrow mimics fieldsets, but I think might be too confusing. However, I do think the cog on a button (vs. 'free-floating' cog) does convey the same action (configuration) with a different type of interaction. I might be a teeny bit surprised the first time I click on it, but the expansion of the mental model (including the difference between a cog and a cog button) is immediately understandable.

I agree we should definitely be careful here, but I think as long as this cog is sufficiently differentiated from the one in block hovers, the mental models hold. We just need to be consistent with which widget we're using for which interaction in all cases.

yched’s picture

re @Bojhan #44
"I think using it like this, whatever way you make it look will not make it more usable - and does trigger a different mental model"

Can you elaborate a little please ? For now I don't know what is this mental model you're talking about, and you don't provide arguments why the current patch would break it.
My point in #43 is precisely that current 2 core use cases for the cogwheel icon are already unsimilar, so I don't see what's the model to break.

On a technical side, I actually don't even see for now how a link-based solution can work.
Even if/when #753270: Impossible to trigger form submit with a link gets fixed (and I admire your optimism about how a solution can surely be "hacked" into something as complex as FAPI / #ajax), AFAICT it won't degrade with non-JS. Without JS, a link is just a link, it cannot participate in a multistep form workflow, which requires form *submission*.
[edit: as fago points in #753270-22: Impossible to trigger form submit with a link, this can be solved by using both a link and a button, with only one or the other actually visible in JS / non-JS].

So I'd like us to be careful and examine options before stating that a UI-pattern that was designed for a very limited use case, involving only presentational, client-side JS (the 'machine name' input) has to be the norm for the ajax-enabled forms that the massive FAPI and #ajax work in D7 finally allows us to build. The two cases just do not compare.

joachim’s picture

> does trigger a different mental model

I am guessing what Bojhan means is that the cog icon means "You are looking at public view of your site, but here is an icon that lets you jump to admin view, edit this piece of the site, and then return here", and that this meaning doesn't apply here at all. #
Though I take your point that (IIRC?) it's also used on the module admin list.

Bojhan’s picture

So I agree that its used confusingly all through Drupal and its definitely not making it better. So let me try to clarify a bit why I don't think it would work here. As opposed to module links and contextual links both provide links to other places, this is specificity about editing something here - I agree Views is similar but as I mentioned previously that might not be the best way to go with this context.

@yched I hope you understand why I am always optimistic about something I see as the best interaction model, that because I am always told its impossible, not accessible, not doable in the time frame.

Any ways lets discuss the interaction, we want to prone the user that you can edit an setting within the table (which is uncommon in any context). Before moving on could you perhaps show a context where there are many configuration items? Now you only show once, I could imagine an image or so to have many formatting settings? One of the things I would imagine we could do to tackle part of that problem is to add a slight background when in editing mode - to that cell.

joachim’s picture

> Before moving on could you perhaps show a context where there are many configuration items?

An image field would probably have:

- image style (ie the size and effects)
- whether or not to link

Then lightbox-type modules would add to this:
- whether to open a lightbox
- what size to show in the lightbox
- what the lightbox image should link to.

My module http://drupal.org/project/nodereference_views would have:
- which view to show
- which display of that view (ugh, which I'd probably have to ajax in... :/

yched’s picture

Examples above are reasonable.

Number field would have :
- number of decimals,
- char for decimal separation (comma, period, ...)
- char for thousands separator (period, comma, space, none...)

As a rule of thumb, I don't expect formatters to have more than 3-4 settings reasonably. More becomes a pain to configure, and are better split into separate formatter types.

I hope you understand why I am always optimistic about something I see as the best interaction model, that because I am always told its impossible, not accessible, not doable in the time frame.

LOL :-)

So I agree that its used confusingly all through Drupal and its definitely not making it better. So let me try to clarify a bit why I don't think it would work here. As opposed to module links and contextual links both provide links to other places, this is specificity about editing something here - I agree Views is similar but as I mentioned previously that might not be the best way to go with this context.

I don't think it's reasonable to say that the cogwheel image is reserved for "links to other places" - this would be a sad hijack of a useful icon, and wouldn't be respected in contrib anyway IMO.
For that matter, I don't agree that this is what the current core use cases convey right now. I do think the "what vs. how" distinction I make in #43 (and that amc formulates in #44 in terms of 'action vs. interaction") is relevant. The cogwheel image itself simply tells you this is dealing with configuration. It's the context surrounding the icon that tells you about the type of interaction that lies behind.

One of the things I would imagine we could do to tackle part of that problem is to add a slight background when in editing mode - to that cell.

Why not, but the default table theming already has row striping to distinguish what belongs to which row, so I'm not sure this is needed ?

amc’s picture

I also disagree that the cogwheel icon can indicate "links to other places." If I'm using a piece of software and I see a disk icon on a button, when I click on it I expect a dialog to open so that I can save my file -- a pretty universal pattern. But if I see a disk icon on a tab, even in the same software, when I click on it I expect a different interaction, namely for a tab to open. Now, by virtue of having a disk icon, I expect that tab to have something to do with saving -- maybe configuring save settings. But whether I expect a dialog box or a tab is dependent on the icon's context -- not the icon itself. Icons are great at conveying what you're trying to do -- think of the icon sets in the Windows Control Panel or OSX dock -- but poor at conveying what will happen when you try to do it. That's conveyed by where I find the icon.

I should say, however, that I think whether it's respected in contrib isn't really relevant. All we can do is carefully construct our UI patterns in core. If contrib authors violate those patterns in their modules, there's not much we can (or, arguably, should) do about it. Things like style guides can help communicate the patterns (and the intention behind those patterns), but it's up to contrib authors to follow the conventions. The point is well taken -- and undoubtedly true, for that matter -- but the fact that people may mess stuff up later doesn't mean we should be willy-nilly in our construction of them in the first place.

Bojhan’s picture

Al right I am happy to know that we cover our edge cases. Sadly I do still disagree with your notion that it will not cause confusion, I feel it will - its a widely abused icon and applying it here will not enhance this. Honestly we can talk about icons all day and not get anywhere, its a subjective topic and highly misunderstood from a cognitive perspective. I think what is clear from our discussion is that clog is simply an icon that will do "something" depending on the context. That's what I am trying to avoid, I want to have a clear expectation what will happen what you click a clog, not after you clicked the clog.

Then again I am not offering any other solution other then [Edit] (/me puts in an image with [edit] to hack around the technical inability) - so lets just move on and see whether we can revisit this issue later on?

I don't think the default table row striping is enough, this because a table like this is putting quite a mental strain on the user - we should help whenever we can.

tstoeckler’s picture

While I do not want to hold up the theoretical discussion in any way:
I just used Firebug to hack the image button to point to a non-existant image, which made the Alt text show up: "Edit".
With a little CSS, I made it look just like the content type thingy and clicking on Edit actually worked perfectly.
I don't know how many (and how horrible) cans of worms we would be opening with such a hack, but I thought it would be worth posting in case of the slight possibility that it might be feasible.

yched’s picture

Thanks for the finding :-), but I don't think that's a viable solution :
- Sounds like a hacky way to design
- The result in other browsers than FF is uncertain
- Pointing to a non existent image triggers a full drupal bootstrap to generate a 404.

Jeff Burnz’s picture

Aside from the bikeshedding around the appropriateness of an icon what is the state of this patch - when I apply it the only useful setting I seem to get is the ability to change the teaser length. For images I get the very strange "Foo: 1" which I have no idea what this means or is meant to do. Decimal fields get no configuration options.

Can someone summarize what the intention is and how this is meant to work, I don't quite get it at the moment and thus am unsure of how to actually test this.

FWIW I think the icon is just great, indicates there's something to configure, seems pretty clear to me.

aspilicious’s picture

Foo is just example of what can be done.
You can add aditional options by implementing the hook.

/**
+ * Implements hook_field_formatter_settings_form().
+ */
+function image_field_formatter_settings_form($field, $instance, $view_mode) {

This for example, implements the foo setting you're seeing.

If this gets committed, we can think about what options have to be added.

yched’s picture

@Jeff Burns:
Yes, the intent of this patch is "only" to provide the UI scaffholding, not to implement actual settings for the various formatters on the various field types. Those will need to happen in separate issues, because they potentially involve far too much bikeshedding.
The patch comes with a couple implementations for a couple formatter types, just so that we don't design in the void. They will need to be removed before the patch gets in.

yched’s picture

So, what's needed to get this in ?

- Settle on the visual element for the 'Edit' trigger, within the limits of what's currently possible with #ajax API (an image button).

current :
formatter_settings_button1.png

alternate proposal 1:
formatter_settings_button2.png

alternate proposal 2:
formatter_settings_button3.png

- Add a background on the cell when in editing mode, as per #48
Done in this patch - the colour could possibly be tweaked, but I can't say I'm thrilled :-). See the screenshot.
Note that the yellowish tones are already used for 'changed settings' and tabledrag's own row coloring.
Introducing another colour in the screen looks off IMO.
I still don't really see the need, to be honest. I mean, inline text got replaced with form inputs, that should make it clear that we are in 'editing' mode ?

- Get a code review

Updated patch polishes a couple alignments.

Jeff Burnz’s picture

@56, 57, thanks people, good explanations.

moshe weitzman’s picture

I reviewed the code and I only found one typo below. I'm OK with the current icon or Alternate Proposal #1. Similarly, I'm ambivalent about the green background on the settings form. All in all, we are in advanced bikeshed now. I'll RTBC after the typo fix.

+++ modules/field_ui/field_ui.js	28 May 2010 21:08:52 -0000
@@ -102,6 +102,19 @@ Drupal.behaviors.fieldManageDisplayDrag 
+    // Custom message with and HTML id.

typo?

yched’s picture

Yes, that was supposed to read "Custom message with an HTML id".
Fixed the typo and expanded the comment a bit.

As I wrote in #57, the example settings for image fields ('Foo' in the patch) will need to be removed before this gets in. I created #812688: Organize image formatters around settings to discuss the optimal way to reorganize image formatters.

yoroy’s picture

I'm getting a "Fatal error: Unsupported operand types in /Users/yoroy/Sites/d7/modules/field_ui/field_ui.admin.inc on line 673" with the patch in #61

yched’s picture

re #62 :
Odd. Can you make sure you empty your sites caches ?
Is that with a default install, or are there contrib modules enabled ?

yched’s picture

Yoroy : if clearing the caches doesn't fix it, could you try with the attached debug patch (requires devel module), and post a screenshot ? (with the krumo dump expanded)

yoroy’s picture

Hmm, apologies, can't reproduce. Installed a fresh head, no more errors.

yched’s picture

Right, that's probably because #553298: Redesign the 'Manage Display' screen requires a fresh install.

yched’s picture

That green background for the 'currently edited settings' is just too ugly.
Replaced it with a dashed border surrounding the formatter settings subform (screenshot)

That's if we really want something to distinguish that subform. Other proposals welcome, but as I wrote at the bottom of #50, just keeping the table striping is good enough as far as I'm concerned (screenshot from #40).

I also make a plea to keep possible visual adjustments or bikeshedding for followups. After this gets in, we need to work on followup patches to take care of actual formatters in #812688: Organize image formatters around settings, #504564: Make summary length behave with fields...

aspilicious’s picture

For me it's obvious if you click on the icon and there appears a form on the same place that those two are related. No need for a colour or a border. But maybe other users need it.
Anyway, I don't think the border is a good solution (if we need a solution).

joachim’s picture

Having two save buttons is really confusing -- even though I've read this thread and I know how the UI works and I saw the yellow box and the warning, I still thought I'd saved it :/

Perhaps change the one in the AHAH to 'Update'?

And the dotted line .... ugh. This is how really bad UI is made -- last minute hasty decisions :(

What is the purpose of the dotted line -- what does it tell me? Do we even need something there, since the form appears as a result of my action?

joachim’s picture

PS. I get this crash when I go to look at comment display settings:

Fatal error: Unsupported operand types in /Users/joachim/Sites/7-drupal/modules/field_ui/field_ui.admin.inc on line 673

yched’s picture

re #70: That's the same as Yoroy's #62. Did you reinstall HEAD after #553298: Redesign the 'Manage Display' screen got in ?

joachim’s picture

I hadn't, but now I have, and yup, it was the same as #62: all fine now.

yoroy’s picture

FileSize
129.05 KB

Maybe something like this; expand and show the setting forms *below* instead of on the right side:

joachim’s picture

That looks a lot better!

I like the cog to the far right of everything. I found when I was testing I kept wanting to click on the text 'Foo' rather than the cog.

aspilicious’s picture

How many settings can a field handle?
What happens with the UI when there are multiple settings?

yoroy’s picture

joachim: yes, I clicked 'Foo' as well. Also, this placement is more consistent with other uses.
We'll have to work with real settings to see if, and how, we show configuration summaries in the normal 'collapsed' state (screen 1 in the mockup). I suspect it'll be hard to make that work/worthwhile.

Depending on the (average) amount of settings for a field we could look into literally implementing the contextual links pattern and make a click on the gear show a little dropdown with links to specific config forms.

Gotta say this "has to be an image" button requirement is weird, but you all knew that already eh? :)

yched’s picture

How many settings can a field handle?
What happens with the UI when there are multiple settings?

See #50 : "As a rule of thumb, I don't expect formatters to have more than 3-4 settings reasonably. More becomes a pain to configure, and are better split into separate formatter types."
So you can have a subform with 3-4 input elements (textfield, checkbox, dropdown select, whatever makes sense)

Depending on the (average) amount of settings for a field we could look into literally implementing the contextual links pattern and make a click on the gear show a little dropdown with links to specific config forms.

Shouldn't be needed IMO, settings should stay simple enough. Besides, that would require some API changes to have each formatter expose their various setting sub-families. Overkill IMO.

About placing the cog icon to the right :
You have to figure a workflow where you select the formatter type, then configure its settings. Having the cog to the far right is not optimal IMO because :
- it forces you to go back and forth with your mouse between left and right
- on a cognitive level, there is a tight relation between the 'pick a formatter' select dropdown and the cogwheel. You configure the settings of a given formatter. Select a different formatter, you'll have different settings available [edit: and some simple formatters won't have any setting. e.g text field 'Default' and 'Plain text' formatters]. The more the two are distant, the more you lose that connection.
Displaying the subform below the formatter select is possibly interesting, but the cogwheel should stay near(ish) the select.

About displaying a summary of the settings :
Yes, it's an absolute requirement. You must have an immediate view of which fields use which formatters with which settings from the 'Manage display' overview, without forcing the user to open each settings subform to see what's inside.
Typically, image fields would have an 'image' formatter, with the image style being a setting. You need to see which styles are used where at a glance.
It's up to each formatter to provide the summary string, and thus to decide if every single one of its settings make sense to be displayed as part of the summary and how, but there has to be a summary.

Gotta say this "has to be an image" button requirement is weird, but you all knew that already eh? :)

It currently has to be a "button", not necessarily an "image button". An image adds less visual bloat to the form, and saves screen real-estate (you don't know how long is the 'Settings' string once it's localized in a random language). It's also more straightforward IMO but I guess that can be argued.
As I proposed in #58, it can also be a more "buttony" version (screenshot over there), although personally it's not my favorite option.

yched’s picture

Updated patch as per #68 / #69 :
- renamed 'Save' button to 'Update'
- removed the dashed border
screenshot (same as in #40 - patch has better CSS visual alignment)

I'm happy that this discussion takes place and involves the right people, and I'm convinced it will result in something neat. I'll simply restate what i wrote in #67 : it would be good if we agreed on a 1st acceptable RTBC state quick, commit the API and current UI scaffolding, and work on refining the UI layout (purely HTML and CSS at this point) in parallel with the patches to add actual settings to actual formatters for existing field types. Working on actual use cases will also help the UI work, but this cannot take place in one single patch.

yoroy’s picture

- on a cognitive level, there is a tight relation between the 'pick a formatter' select dropdown and the cogwheel. You configure the settings of a given formatter. Select a different formatter, you'll have different settings available. The more the two are distant, the more you lose that connection.

Displaying the subform below the formatter select is possibly interesting, but the cogwheel should stay near(ish) the select.

'Configure the settings of a given formatter' is too specific for how this will be interpreted I think. It'll be seen as configuring the field, i.e. the entire row in the table. Yes, what you can configure is dependent on the formatter but that's of secondary importance. The gear should be far right because that's where all over core we put our operations on a given object, we want to be consistent with that pattern.

Summaries is fine, can't design for that in a void though, so that's for a followup imo.

yched’s picture

re #79 - makes sense.
"it forces you to go back and forth with your mouse between left and right" is still true, though ?

yoroy’s picture

I would aim to make as much of the table cell as possible clickable for config, not just the gear alone.

yched’s picture

Assigned: yched » Unassigned
FileSize
21.67 KB
38.54 KB

I would aim to make as much of the table cell as possible clickable for config, not just the gear alone.

Not really doable, since the cell is where the summary text is. If that's clickable, then it's a link, and not doable currently :-).

Updated patch floats the cogwheel to the right.
I still feel the horizontal mouse distance between this and the formatter select is going to be a real pain for large screens, especially when overlay is disabled. Screenshot taken on my 15'' laptop - not an unusual setup.

Kept the form settings in the right cell for now. Since the summary is displayed there, displaying the form in a different place makes little sense IMO.
+ displaying the form below the formatter select makes it as if the select belonged to the scope of the 'Update / Cancel' buttons, which is not good.

yched’s picture

Assigned: Unassigned » yched

easier tracking

yoroy’s picture

yoroy’s picture

fieldformattersettings

aspilicious’s picture

1) I see a small problem with yoroy's design.

That design looks ugly if the update button and configure button aren't the same size.

Solution
You could say hey no problem lets find a good number for the width for both buttons.

Problem
What if some translations make the string bigger than the chosen width?

2) What happens if those descriptions exceed the width of their column?

yched’s picture

FileSize
109.4 KB

re #86 / Yoroy's #85 :

- Yes, screen real estate is the reason why text buttons are problematic. We cant put them here, they eat an unpredictable amount of horizontal space reserved for the summaries.

- all of core put 'Submit' buttons below forms, following the natural course of eye and mouse focus when filling the form.
with #85 this becomes
formatter_settings_87.png
This is a usability loss IMO, and the benefits are not clear to me.

- Having a button that changes it's nature ('Edit' -> 'Update') is not found anywhere else either.

yched’s picture

OK, trying a subtle change of gears (pun intended).

What if we put the focus on the 'edit in place' metaphor instead of the 'configure' ? After all that's what this UI does : you have a string summarizing settings, and you switch to in-place editing of those settings.

So instead of a cog, the image would be a pen or a 'pen on a notepad'.
Does that make having the icon just to the *left* of the summary more acceptable ? (like in this shot, which is still the best layout so far IMO - no actual pen icon under my belt, sorry)

amc’s picture

Assigned: Unassigned » yched

If we're looking for other icons, there's #606490: Drupal GPL icons - a softfacade initiative. I think the general iconization of D7 stalled some time ago, but the icons should be available to use. Take a look and see what you like.

yched’s picture

FileSize
6.59 KB

So here's a mockup using the pen icon from yoroy's http://drupal.org/project/protocons :

formatter_settings_90.png

yched’s picture

FileSize
7.14 KB

buttony version :

formatter_settings_91.png

yched’s picture

Adjusted CSS a bit, and added more dummy settings to image formatters, to visualize longish summaries better.
I also found the image button without a surrounding 'button' look makes a narrow mouse target in practice. Re-added default button style.

So, updated screenshots with the 3 steps :

1) either a cogwheel icon, which then needs to be floated to the right to match the rest of core
screenshot

2) or a pen icon, that might allow us to keep the button closer to what it configures and reduce horizontal mouse span.
screenshot

What is it going to be, folks ?

yoroy’s picture

- I just don't get why put the (potentially) largest forms into the smallest possible space. Note that none of our examples use descriptions which is very likely to be the case. Literally painting ourselves into a corner here.
- We can't put an edit icon *before* an element. An icon before a label suggests it's a visual label. It has to be to positioned after the text to communicate its action/button function. Also, it's not positioned closer to what if configures, it's paired with the summary, which is not the actual thing you configure here. As I pointed out earlier, this interaction is very likely to be interpreted as simply configuring a field (not it's formatter) as a whole. floating to the right is the best option then.
- We don't have an interaction like this in core (besides the machine name one already pointed out) because we haven't solved this ui problem yet. It's a new problem. In general its good to be consistent with what we have but we can't solve this particular problem with what we have. That's where the consistency argument should be dropped and some room for innovation given.

yoroy’s picture

Anyway :)

Variations on form placement, button placement and how the summary could be presented.

yched’s picture

FileSize
22.8 KB

[edit : oops, crosspost with #94]

I just don't get why put the (potentially) largest forms into the smallest possible space. Note that none of our examples use descriptions which is very likely to be the case. Literally painting ourselves into a corner here.

OK, I missed that point in your proposal.
screenshot with the settings form expanded below the formatter select.

I do disagree with the arguments against the edit button next to the summary, and fail to see how #92-2) can be confusing or unclear.
I won't block the issue on this, though. I'll leave others feedback if they want.

In general its good to be consistent with what we have but we can't solve this particular problem with what we have. That's where the consistency argument should be dropped and some room for innovation given.

Fully agreed :-). What I meant in #87 about the combined 'Edit / Update' button was that it breaks consistency with no real benefit AFAICS, but actual drawbacks (more mouse movements).

yched’s picture

re #94:

3a) would add needless height to all rows, making the form tedious to handle (reducing by half the # of rows visible in a viewport height), while 3 lines of summary in the right cell will be more than enough for 95% formatters wi (*if* a text button doesn't steal horizontal space :-p)
3b) it is.

2) : I'm not sure how this is doable in the context of a table - short of having the part with the settings form be a separate row, which would play bad with tabledrag...

yoroy’s picture

And I don't even like sugar in my coffee! :) I too would love some reviews from others.

yched’s picture

Investigated a little. Spanning the settings over an entire new row would require:

- a way to disable tabledrag while in 'edit settings' mode (including making visually clear that it's disabled)

- patching theme_table() to allow more flexible striping (the 'settings' row needs to have the same striping than the main row for the field).
Even though this table is currently themed using a dedicated template for now, #616240: Make Field UI screens extensible from contrib - part II will need to switch it back to a regular theme_table().

Frankly, I don't see this happening. So we're back at #94 and this screenshot.

Bojhan’s picture

Interesting, I really like the approach taken. @yched Thanks for being so open in our iterative process.

Looking at #95 and its screenshot its actually already closer to what we thought. I wouldn't say its necessary to add something that inflicts with the table drag. Also regarding your comments on increasing the height of a table row, I would be inclined to agree it would crumble the scanability of the page.

However since we are doing a pretty advanced interaction within an already fairly dense table. We need to be adding some extra help to the user. Yoroy clearly illustrated that, one of the biggest problems is orientation, what think am I changing isn't clear. Adding color, and changing the spacing will help with this.

screenshot-changes-formatter-ui.png

I hope the summary is shortened or something? I would imagine this table becoming unreadable if you have fields with quite some formatting settings? Same goes for vertical tabs, we don't show all summary elements in there it would get to crowded.

yched’s picture

re #99 :
formatters are responsible for what they expose as a summary. It's their responsibility to keep it short and decide what is worth making it in the summary. Frankly, formatter settings should stay simple, IMO.

What do you mean by "loose the fieldset" , Getting rid of it ? It's important to give the scope of the 'Update / Cancel' buttons. They don't involve the formatter select.

Will address the other points tomorrow.

yoroy’s picture

yeah, get rid of the fieldset, it adds visual clutter and the exact scope isn't really that important (main orientation for the user is the whole row).
Agreed that #95 is a huge step in the right direction. thanks!

amc’s picture

Regarding mouse movements: certainly, we want to minimize the work for the user, but let's not forget that these are generally set-and-forget settings. The table's scannability is enhanced by having the settings button out of the way, and is more important than the convenience of having it closer.

Regarding color: I agree win Bojhan that blue would be a good, neutral color to indicate editing.

Regarding text: Do we really need the "Settings for..." text? I think if the field and label fields are aligned at the top and the settings are left-aligned with the format field it should be clear enough what I'm editing.

yched’s picture

andypost’s picture

FileSize
12.76 KB

I think it's better to replace the formatter select box with a some caption when editing options for current formatter (like proposed in #99).

It's more intuitive to user to understand that select-box does not used in settings for formatter!

yched’s picture

re #104 : that was the intent of the fieldset in #94, but it was deemed not needed.

If we go that way, we really lose the fact that we're configuring options for the selected formatter. -1

yoroy’s picture

We're at a point where mockups won't help us evaluate the experience anymore. Ideally we start with the minimal patch and add stuff where we feel it is missed instead of adding it all in and argue about what could be removed.
Or yched codes both approaches but I don't want to ask that from him :)

moshe weitzman’s picture

I propose that we commit what we have and iterate from there. We're pushing our luck with yched here :).

Lets remove the demo code for image formatter and then i'll rtbc.

Bojhan’s picture

@moshe This stage in core development we shouldn't be pushing our either luck on that issues get fixed after commit. From my experience working on loads of UI issues, it hardly ever gets attention after.

yched’s picture

[edit: crosspost with #108]

Added a couple minor fixes in the ajax effects.

Here's the patch without the demo code for text fields (that should be done in #504564: Make summary length behave with fields) and image fields (#812688: Organize image formatters around settings).
It only implements the UI structure, and therefore has no visible effect on the current UI.

The other patch (.txt) includes the demo code, to have something to play with.
Screenshot is the same as #103

joachim’s picture

> This stage in core development we shouldn't be pushing our either luck on that issues get fixed after commit. From my experience working on loads of UI issues, it hardly ever gets attention after.

It's not just UI. Remember the 'Oh let's commit MENU_LOCAL_TAB even though it's a DX WTF and bikeshed the name later?' Beware of postponing bikesheds -- sometimes they never get built at all.

yched’s picture

It's also important to get something in soonish so that the work on followups can start. The API part hasn't moved since the 1st version of the patch in #9.

+1 for not rushing something unfinished, but +1 for not bikeshedding over labels :-)

So, are there identified blockers on the current state of the patch, and if any, how can they be solved ?

I can try to work on a working version for the proposal in #104 if that's deemed important, but all of those take me non negligible time, so there's only a limited amount of shots :-)

Bojhan’s picture

@yched Yes, I think that would be great. Presuambly you can't change the dropdown anyways? So we need to remove it eitherway. Lets aim in terms of label for something like, Formatter settings: Image linked to content

Also we are simply doing a few iterations here and as far as I see we are getting to a better design. This is totally not bikeshedding, this is simply designing, going back and forth on ideas.

You could have had the API in, if you made an API for UI for Field formatter settings :P issue.

I think when we have #103 it is 95% there, yoroy might have some discussion points here though.

yoroy’s picture

Status: Needs review » Needs work

I'm quite happy with how far we've come now. My remaining points:

1. I'd like to use the blue tint Mark Boulton proposed for another 'editing' context: http://skitch.com/yoroy/dgcuu/d7-5-editcontent-v2b.jpg
Value: #d5e9f2

2. Toggle between these two screenshots to see the 'hide select list' trick in action:
http://skitch.com/yoroy/dgpn5/untitled
http://skitch.com/yoroy/dgpdn/untitled
It makes sense to hide the select list when editing its settings, looking at how the patch-with-demo-settings in #103 works, it introduces a WTF moment where when you choose another formatter the settings box collapses automatically. Which is correct of course, since you'r basically cancelling the settings form, but it would be much better to remove the possibilyt to change the formatter when configuring. It's ok for the interface to take the lead a bit here and 'protect you from yourself' by hiding something you probably don't want to do right now.
Bojhan's suggestion to put this text there is good:
"Formatter settings: Image linked to content"

3. Not sure we need the border around the gear icon. Also misses a hover state. I'm fine with moving that to a follow up though.

So, needs work for 1 & 2.

Gotta love what Firebug lets you do for prototyping (not quite the right text labels):
formatter ui

yched’s picture

Status: Needs work » Needs review
FileSize
22.36 KB
26.93 KB

Patch implementing the proposal in #104 - screenshot
To compare with patch in #109 - screenshot

As I wrote above, I don't support this option. It looses the connection with what you're configuring IMO. + The label looks off, it's not a table header, it's not a fieldset... We have no familiar styling for this.

Presuambly you can't change the dropdown anyways?

You can. In patch #108 this sets back the row to 'summary mode' (for the newly selected format).
I actually think it would be more handy and more usable (less clicks) to stay in 'edit' mode (for the newly selected format). Lets you browse the formats quickly and see the settings each one offers.

You could have had the API in, if you made an API for UI for Field formatter settings :P issue.

Heh, the API has been in for months :-p, but without any UI actually calling those hooks, it was useless and thus remained unused, and got removed just a couple weeks ago.

yoroy’s picture

You can. In patch #108 this sets back the row to 'summary mode' (for the newly selected format).
I actually think it would be more handy and more usable (less clicks) to stay in 'edit' mode (for the newly selected format). Lets you browse the formats quickly and see the settings each one offers.

Or stay in edit mode: yes. lets you explore options. Going back to summary mode on changing the dropdown is confusing.

Bojhan’s picture

@yched I can understand your feeling that with the absence of the actual form element you loose connection.

What I am arguing is that you actually create more connection. Why? First of all there is less cognitive friction understanding that a label like "Format settings: lalala" is the subject of the options below, where as understanding that the form element is the subject of the options below is somewhat less normal.

As much as I try to understand, I do not see a significant way to tackle the problem of a form element being the sole subject other then a label. So I don't think a dropdown will help usability, it will help efficiency but I do see orientation problems will occur at first.

Closing to summary is obviously a wrong thing to do, but that's more a side effect of a feature I am arguing to remove.

andypost’s picture

As for me it's more intuitive #114. And Bojhan give a good explanation why!

understanding that the form element is the subject of the options below is somewhat less normal

yoroy’s picture

Thanks for that explanation bojhan, agreed that hiding the dropdown is the better option.

joachim’s picture

I was thinking hiding the dropdown would be clearer like Bojhan says, but this:

> I actually think it would be more handy and more usable (less clicks) to stay in 'edit' mode (for the newly selected format). Lets you browse the formats quickly and see the settings each one offers.

is compelling!

+1 for the blue.

yoroy’s picture

Yeah, it's tempting but that's optimizing for the expert user. This is a really complex interface and at this stage, quite deep into the ui, it would be better to actively guide users by 'consolidating' stuff instead of opting for flexibility.

yched’s picture

From my #114 on the 'remove the select' version (screenshot)

The "Format settings: foo" label looks off, it's not a table header, it's not a fieldset... We have no familiar styling for this.

If we go this route, I'm not sure how we address that. In patch #114, the styling (bold, uppercase) is applied in field_ui.css so that it 'looks like' other headings in seven - but will look plain broken in other admin themes.

The corresponding styling would need to be applied in seven's style.css, but I don't see us adding some css for 'this specific page in field UI'. It needs to be formalized into a know, identified form UI pattern (like 'fieldset', 'table header') that any good candidate admin theme needs to style.

I also don't see us using a h2 / h3 tag in there, I don't think we use those in forms so far.

yched’s picture

Oops, I only notice now that my #114 was a crosspost with yoroy's #113, which I completely missed until now.
If the styling for the label proposed in http://skitch.com/yoroy/dgpdn/untitled is OK for everyone, I can work from there.

aspilicious’s picture

For me it looks great :)

yoroy’s picture

yep, go with a styling that matches a form label (bold)

Bojhan’s picture

Yup, good!

mfb’s picture

Subscribe. I'm upgrading a CCK field module and this works very nicely.

yched’s picture

Updated patch for #113 :
- includes the suggested shade of blue
- displays the legend with bold text.
- fixed a bug when dragging a row to the 'hidden' section while it is in edit more. The code that handles this in field_ui.js requires the presence of the 'formatter type' select, so I include it in all cases, and just hide it through CSS when the row is in edit mode.
We also need it in for when the global form is submitted while a field is in edit mode, for that matter.

Screenshot

Still need to polish tabledrag's 'Changes made in this table will not be saved until the form is submitted' message flashing in and out when dragging a row into / out of the 'hidden' region. Working on it. Patch can be tested and reviewed meanwhile.

Not sure we need the border around the gear icon. Also misses a hover state. I'm fine with moving that to a follow up though

I initially preferred the cog-only version without the buttony surrounding, but in practice it makes an annoyingly narrow mouse target.
Hover state : can only involve the background, not the icon (unless done in JS - that's changing a DOM attribute, not a CSS property). But in this case, all buttons in Seven miss an hover background too ?

yched’s picture

Fixed the issue with tabledrag warning message. It's now preserved when the table is refreshed through AJAX.

So this should be ready. I'll roll a patch without the demo settings form when this is marked RTBC.

Screenshot is the same as #127.

yched’s picture

Fixes a minor vertical misalignment for updated summaries.

aspilicious’s picture

the place for the blue busy icon is a bit disturbing for me (between update and cancel).
Besides of that it looks good and works great.

yched’s picture

#130 : true, but that's a bug in ajax.js, not for this thread to fix.

yched’s picture

Shameless plug : while working in this, identified a couple missing commands in the ajax framework - the current patch doesn't need them in the end, but would be good to have still. Patch in #818660: Prevent duplicate code by adding an AJAX command to invoke simple jQuery methods

yched’s picture

Cleaner way to keep track of tabledrag's dragged rows (through a hidden form element rather than through a cookie in #129)
Side bonus : AJAX updates preserve the '*' markers added by tabledrag on dragged rows.

yched’s picture

anyone ?

joachim’s picture

Status: Needs review » Needs work

Overall the UI feels great. :)

At admin/structure/types/manage/article/comment/display, switching from 'Default' to 'plain text' seems a bit odd, because you get an ajax spinner which then results in nothing at all. It is possible to disable that if nothing's going to appear?

The blue for the table row editing is nice. So far it's unique in the UI, and I can't think of anything else that might use it. But you know what happens when someone says that.... in 3 months some contrib module or other will find a use for it ;)
So could we give the CSS class a more generic name than 'field-formatter-settings-editing' perhaps?

The change I make to the comment field trimmed length doesn't get stored when I change, update, save.

Because of this I can't tell, but what happens if I am lazy and just click save instead of update first?

yched’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
30.37 KB

switching from 'Default' to 'plain text' seems a bit odd, because you get an ajax spinner which then results in nothing at all. It is possible to disable that if nothing's going to appear?

ajax.js doesn't allow that. Either a select (or any other input) triggers an ajax request on 'change' event, or it doesn't. There's no way to insert custom logic in between and say 'no wait, don't trigger the request in this case or that case'.
Even if we could, this would add much complexity because we'd need to update the HTML anyway, but with client-side code instead of receiving new HTML from the server. We don't want to go there.

So could we give the CSS class a more generic name than 'field-formatter-settings-editing' perhaps?

That's what tabledrag does: uses a yellow background with class 'drag-previous'. On the contrary, giving a more generic class name like 'row-editing' could cause unexpected clashes. It's very possible that we could be smarter here and formalize a couple things, but this requires a more general reflexion to define generic UI patterns, and could also very much derail the issue. Not in the scope of this patch.

Updated patch, fixes settings not getting actually saved. Confusion between array_intersect_key() and array_intersect_assoc() :-(.

what happens if I am lazy and just click save instead of update first?

It works as if you clicked 'update' first. You can be lazy :-).

moshe weitzman’s picture

I'm ready to RTBC this as yched has replied to all concerns. But it looks like example code is in the image formatters.

moshe weitzman’s picture

Status: Needs work » Reviewed & tested by the community

On second thought, the CVS committers might appreciate that example code so lets send it to them as is.

yched’s picture

Patch for commit - i.e without the demo code in text (that should be done in #504564: Make summary length behave with fields) and image fields (#812688: Organize image formatters around settings).
As is, the patch only implements the UI structure, and therefore has no visible effect on the current UI.

Patch with demo code can be found in #136.
Screenshot (same as in #127)

Status: Reviewed & tested by the community » Needs work
Issue tags: -Needs usability review, -Usability, -ui-pattern

The last submitted patch, field_ui_formatter_settings-796658-139.patch, failed testing.

amc’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, field_ui_formatter_settings-796658-139.patch, failed testing.

tstoeckler’s picture

Status: Needs work » Needs review
Issue tags: +Needs usability review, +Usability, +ui-pattern
yched’s picture

Status: Needs review » Reviewed & tested by the community

Testbot hiccup. Now green, back to RTBC. Thks for the retests, folks :-).

yoroy’s picture

Testing the patch on a clean install: I don't see any settings form show up anymore. I see a blue spinner but no form showing up. Can somebody verify please.

yched’s picture

Yoroy : Are you testing the patch in #139 ? It has all the demo hook_field_formatter_settings_form() implementations removed :-)
The patch in #136 has the demo code, and it works fine on my setup.

yoroy’s picture

Status: Needs review » Reviewed & tested by the community

Duh, sorry :)

yched’s picture

Cool :-).

To avoid the confusion for other people, I'm reposting both patches (hope the bot won't choke this time)

- "...136.demo.patch.txt" includes dummy settings for image formatters and text 'trimmed' / 'summary or trimmed' formatters, so that there is actually something to play with.
Requires clearing site caches after applying.

- "...139.patch" is the actual code to commit. Without the demo code, and with currently no formatter having settings forms in core, the patch in itself has no visible effect, it only provides the UI scaffolding. Actual settings for actual formatters is a job for other threads : #504564: Make summary length behave with fields, #812688: Organize image formatters around settings...
Requires cleaning site caches if you've tested the 'demo' patch beforehand.

Screenshot (same as in #127)

See #3 for why formatter settings are important. Meanwhile, I stumbled on http://drupal.org/project/hover_preview for a perfect illustration - 2x(n^2) formatters where n is the # of imagecache presets. Ouch.

yched’s picture

Issue tags: -Needs usability review

bump, + the usability review was done :-)

Bojhan’s picture

/me disables

Manuel Garcia’s picture

sorry for subscribing but well, I'm planing on porting galleryformatter to d7 and this is critical for it.

andypost’s picture

Gallery API also waiting this patch to be commited.

sun’s picture

Whew, quite a patch. Would love to see technical in-depth JS and AJAX reviews and/or approvals here.

Manuel Garcia’s picture

I've tested the patch porting the galleryformatter module, just for testing/learning, it works great, I haven't found any problems so far with it.

yched’s picture

re #153 : reviews more than welcome, but we're getting dangerously close to beta1, and I'd really hate to go another release without formatter settings while the thing is available :-).

yched’s picture

Also, note that the ugly field_ui-display-overview-table.tpl.php and associated template_preprocess_field_ui_display_overview_table() (they're already ugly in current HEAD, patch makes them a tad uglier) will go away with the second part of #616240: Make Field UI screens extensible from contrib - part II. But we need to get this sucker in first.

EvanDonovan’s picture

Subscribing to get back to, but don't know whether my level of knowledge will be helpful. Would love to see these settings though.

Crell’s picture

Subscribe. I don't know that I have bandwidth or competence to review this, but I at least want to track it. :-) I will try to find review time, but don't let that stop committers!

bleen’s picture

RTBC +1

I installed the patch on a clean d7 install and everything seems to work as advertised. I changed the body format to trimmed and set the length to 500 ... I played with a couple of the image formats for a while and then I undid all my changes.

Very cool

Manuel Garcia’s picture

If anyone wants to test this out with a working formatter which uses settings, you can use the 7.x dev version of gallery formatter which I have just branched in CVS:
cvs -z6 checkout -d galleryformatter-DRUPAL-7--1 -r DRUPAL-7--1 contributions/modules/galleryformatter/

sun’s picture

Status: Reviewed & tested by the community » Needs work

This patch definitely needs to get in for D7. As I'll mention further below, I think it would be wise to split the technical API change from the totally cosmetical tableDrag UI tweaks in this patch; the latter into a separate issue. I.e., the agreed on UI to change/submit field formatters and formatter settings should be kept here and committed as-is, but almost half of this patch deals with additional tabledrag.js adjustments to provide a nice user experience when changing field formatters... only those parts are slightly questionable from my perspective, and therefore, we should deal with those in a separate issue. Most remarks in the review below are about those tableDrag changes.

+++ misc/tabledrag.js	7 Jun 2010 14:17:45 -0000
@@ -451,7 +451,7 @@ Drupal.tableDrag.prototype.dropRow = fun
       self.rowObject.markChanged();
       if (self.changed == false) {
-        $(Drupal.theme('tableDragChangedWarning')).insertBefore(self.table).hide().fadeIn('slow');
+        self.rowObject.addChangedWarning();
         self.changed = true;
       }

+++ modules/field_ui/field_ui-display-overview-table.tpl.php	7 Jun 2010 14:17:45 -0000
@@ -16,35 +16,54 @@
+  <?php if ($changed_warning): ?>
+    <div id="field-settings-changed-warning" class="warning"><span class="warning tabledrag-changed">*</span>&nbsp;<?php print t('Changes made in this table will not be saved until the form is submitted.') ?></div>
+  <?php endif; ?>

+++ modules/field_ui/field_ui.js	7 Jun 2010 14:17:45 -0000
@@ -102,15 +102,37 @@ Drupal.behaviors.fieldManageDisplayDrag 
+    // Custom message with an HTML id so that addChangedWarning() can avoid
+    // duplicates.
+    Drupal.theme.prototype.tableDragChangedWarning = function () {
+      return '<div id="field-settings-changed-warning" class="warning">' + Drupal.theme('tableDragChangedMarker') + ' ' + Drupal.t('Changes made in this table will not be saved until the form is submitted.') + '</div>';
+    };

1) The custom code in field_ui.js seems to try to do the same as tabledrag.js already does - testing whether anything in "tabledrag.changed" already, and if not, .addChangedWarning(). Not sure why we need to duplicate this functionality? If the existing code does not account for a potentially already existing changed warning, then it would make more sense to fix tabledrag to properly account for it by default.

2) Depending on 1), if, for some reason, we need to output the changed warning on the server side already, then it needs to be rendered through a dedicated theme function (instead of raw HTML in a template). However, I'm still not sure why the warning is output on the server side here.

However, see also below.

+++ misc/tabledrag.js	7 Jun 2010 14:17:45 -0000
@@ -1056,6 +1056,10 @@ Drupal.tableDrag.prototype.row.prototype
+Drupal.tableDrag.prototype.row.prototype.addChangedWarning = function () {
...
+}

1) Missing JSDocBlock.

2) Missing closing/trailing semicolon after function declaration.

+++ modules/field_ui/field_ui.admin.inc	7 Jun 2010 14:17:45 -0000
@@ -626,13 +645,121 @@ function field_ui_display_overview_form(
     $table[$name]['type'] = array(
       '#type' => 'select',
       '#options' => $formatter_options,
...
+      '#field_name' => $name,
+      '#op' => 'change_format',

AFAICS, the #op of the select list should actually be 'change_type'.

+++ modules/field_ui/field_ui.admin.inc	7 Jun 2010 14:17:45 -0000
@@ -626,13 +645,121 @@ function field_ui_display_overview_form(
+      $settings_changed = TRUE;
...
+    if ($form_state['formatter_settings_edit'] == $name) {
...
+        $table[$name]['#settings_editing'] = TRUE;

Scanning through the patch, there seem to be quite a lot of state tracking properties and variables. I wonder whether all of these couldn't be streamlined into a single $form_state property?

+++ modules/field_ui/field_ui.api.php	7 Jun 2010 14:17:45 -0000
@@ -131,6 +131,63 @@ function hook_field_widget_settings_form
+function hook_field_formatter_settings_form($field, $instance, $view_mode) {
...
+function hook_field_formatter_settings_summary($field, $instance, $view_mode) {

Both hooks should get $form and most importantly $form_state passed.

+++ modules/field_ui/field_ui.css	7 Jun 2010 14:17:45 -0000
@@ -16,10 +16,54 @@
+.field-display-overview .field-formatter-summary-cell {
+  font-size: 11px;
+  line-height: 13px;
+}
...
+.field-display-overview .field-formatter-settings-edit {
+  float: right;
+  margin: -3px 0;
+  padding: 1px 8px;
+}
...
+.field-display-overview tr.field-formatter-settings-editing {
+  background: #D5E9F2;
+}
...
+.field-display-overview .field-formatter-settings-edit-form .form-item {
+  margin: 10px 0;
+}

This looks like some or most of these styles actually belong to one of the core themes, but not into the module's default, base CSS - which should only contain the bare minimum to make it look acceptable in Stark.

+++ modules/field_ui/field_ui.css	7 Jun 2010 14:17:45 -0000
@@ -16,10 +16,54 @@
+.field-display-overview .field-formatter-settings-edit-form  .formatter-name{

(minor) Duplicate + missing space here.

+++ modules/field_ui/field_ui.js	7 Jun 2010 14:17:45 -0000
@@ -102,15 +102,37 @@ Drupal.behaviors.fieldManageDisplayDrag 
+      // Allow the server to track dragged rows and preserve 'changed' markers
+      // and the warning message through AJAX refreshes of the table.
+      var $input = $('input[name=dragged_rows]');
+      var dragged = $input.val();
+      dragged += (dragged ? '|' : '') + row.id.replace(new RegExp('^row-'), '').replace('-', '_');
+      $input.val(dragged);

Effectively + in the spirit of this issue, I'd be interested in a version of this patch without all of these UI tweaks of marking changed rows and displaying warnings... IMO, we should defer those interface tweaks to a separate issue -- without them, this patch looks very reasonable and ready to fly.

64 critical left. Go review some!

yched’s picture

Basically, the issue is that tabledrag assumes it's the only one to have a use for those messages - thus it only checks its own criteria to determine whether a message should be displayed or not (is this the 1st row being dragged ?), and does not bother whether a message already exists from somewhere else.

I agree it would be best to fix this in tabledrag in a separate patch. But I'm not sure we want to commit the patch without those messages, they address real UI concerns voiced earlier in the thread (without those, it's not clear what you have changed, nor that you need to hit the main 'Save' button before changes get actually saved). If the patch didn't have them, it wouldn't have gotten a green flag from the UX folks.
So I'd advocate for committing the current approach, and cleanup tabledrag so that it's more flexible with its messages in a separate patch.

As per messages on server-side :
- for non-JS mode : whole page is redisplayed - yet "your changes won't be saved until you hit Save' still stands.
- more generally : so that tabledrag warnings don't go away when you edit a formatter's settings.
Agreed that it would need to go in a theme function, though.

I'll try to roll a patch with the other non-tabledrag remarks soon.

yched’s picture

#448292: Drag and Drop for table rows is not accessible to screen-reader users just changed the layout of tabledrag messages by adding the "show row weights" link, so, as advised by sun, here's a patch *without* the "changed" warnings. We do need those in the end, but having them play nicely with tabledrag's own messages will require some work in tabledrag.js that belongs in a separate patch, and cannot block this one, which needs to get in before beta.

- "Scanning through the patch, there seem to be quite a lot of state tracking properties and variables."
Not really (two - one now that messages are gone). Those cannot go in $form_state because they're needed to theme the table, and $form_state doesn't live up to there. They won't be needed anymore when we refactor the theming part in #616240: Make Field UI screens extensible from contrib - part II, but we can only move one step at a time :-).

- "hook_field_formatter_settings_form() / hook_field_formatter_settings_summary() should get $form and most importantly $form_state passed".
Done for hook_field_formatter_settings_form(). Doesn't apply to hook_field_formatter_settings_summary().
I omitted those to stay consistent with the current state of the other 'field settings form' hooks (hook_field_settings_form, hook_field_instance_settings_form, hook_field_widget_settings_form), which currently don't receive $form / $form_state either, but those will need to be fixed separately.

- redispatched CSS properties between field_ui.css (base layout) and seven.css (style)

- fixed the other minor points mentioned in #161.

Like before, two patches :
- "...163.demo.patch.txt" includes dummy settings for image formatters and text 'trimmed' / 'summary or trimmed' formatters, so that there is actually something to play with.
Requires clearing site caches after applying.

- "...163.patch" is the actual code to commit. Without the demo code, and with currently no formatter having settings forms in core, the patch in itself has no visible effect, it only provides the UI scaffolding. Actual settings for actual formatters is a job for other threads : #504564: Make summary length behave with fields, #812688: Organize image formatters around settings...
Requires clearing site caches if you've tested the 'demo' patch beforehand.

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

And once again, we get to RTBC. Quick, lets do this.

yched’s picture

sad panda bumps...

mcreature’s picture

Subscribing

moshe weitzman’s picture

Manuel Garcia’s picture

Just tested "...163.patch" with gallleryformatter cvs -z6 checkout -d galleryformatter-DRUPAL-7--1 -r DRUPAL-7--1 contributions/modules/galleryformatter/, works perfectly, have not found any glitches so far with it.

I do see a problem with usability/workflow though, if you go edit a formatter settings, you change some setting, and click update from there, the user gets the feeling that the setting has been updated. This is not the case however, it only happens when you SAVE the page admin/structure/types/manage/article/display for example. Also, there is no warning to the user letting them know about this situation, so you could change settings, hit update, and wonder off to do something else, not knowing that the settings were not actually saved.

I'm seeing this is not something particular to this thread, nothing warns you that you should save when you change formatter either, so I guess this is something for a new/existing issue about it, so nevermind.

Imho this is rtbc, then again, I'm just a contrib tourist wondering around in the wonderful core world! =))

yoroy’s picture

Thanks Manuel, this is good feedback. The 'have to submit the page to really save changes' pattern is indeed not particular to this issue but a weirdness found in multiple places in core and contrib.

This is one of those non-criticals that are 'really important to haves so core makes life easier for contrib the next couple of years'.

yched’s picture

@Manual Garcia, @yoroy #168 - #169 :
see #161 to #163.
In short : the patch had messages to make that clearer. They require more refactoring in tabledrag.js to allow a cleaner integration, and so were removed in patch #163. They will need to be added back in a followup patch that messes with tabledrag, but this cannot hold this patch back.

Manuel Garcia’s picture

OK, thanks yched for the clarification, I just glanced over the big comment on #161 without actually reading it, my fault.

I agree that this shouldn't be held back just because of this, let's do it!

sun’s picture

bump; we get a deferred dependency here, which is ideally finished before beta, too.

loominade’s picture

subscribing

sun’s picture

Priority: Normal » Critical

I am elevating this patch to critical. Everyone agrees.

yched’s picture

This has been pending commit for almost a month now, not one word from drieschick ?

Given how important this UI addition is, the amount of work that was put in the UX design process by 3+ persons plus myself, and the fact that at least 4 other patches are postponed on this one, this is a little disheartening...

Dries’s picture

Finally had the time to look at this patch. Looked at it for 30 minutes (only), played with the demo and everything looked good for the purpose of this patch. Hence, I committed it to CVS HEAD. Thanks all!

Dries’s picture

Status: Reviewed & tested by the community » Fixed
andypost’s picture

Priority: Critical » Normal
Status: Fixed » Needs work
Issue tags: +Needs documentation

Let's document this, #127

joachim’s picture

Ironically, what brought this to light was #675116: field.api.php hook documentation is incomplete, which removed the documentation for this hook since it wasn't getting invoked ;)

yched’s picture

@joachim : not really :-). A UI for formatter settings has been on my list since the beginning of D7 Field API, it was 'just' a long road getting there (D7 $form / $form_state overhaul, redesign of 'Manage display' screen...)

@andypost : not sure what you think needs documentation ?

@Dries : yay !

Created #857312: Add a "changes not applied until saved" warning when changing widget/formatter settings to tackle 'form needs saving' messages (see #161-#163).
Will also finish #616240: Make Field UI screens extensible from contrib - part II.

+ See you in
#812688: Organize image formatters around settings
#504564: Make summary length behave with fields
#857314: Add form for number formatter settings
Help / patches welcome for those !

andypost’s picture

@yched I mean to drop a line into http://drupal.org/node/224333

yched’s picture

re @andypost : this is not a change wrt core D6 -> D7. Until now, such Field API changes did not get a mention on http://drupal.org/node/224333.

EvanDonovan’s picture

Seems like Field API changes might make better sense to be documented on a page describing how module developers can upgrade from CCK -> Field API. Does such a page exist?

andypost’s picture

Something strange with button for changing formatter settings for field

EDIT: Related Issue #883336: theme_image_button() broken (needs tests only)

yched’s picture

Yeah, I got it in french on my own setup.
I think theme_image_button() got broken in #690980: Disabled form elements not properly rendered

BenK’s picture

Tracking this thread...

chia’s picture

Something must have broken this
When i try to change Format of any field i got the following error

An AJAX HTTP error occurred.
HTTP Result Code: 500
Debugging information follows.
Path: /system/ajax
StatusText: Service unavailable (with message)
ResponseText: Recoverable fatal error: Argument 2 passed to drupal_array_get_nested_value() must be an array, string given, called in /var/www/html/drupalcvs/includes/form.inc on line 1007 and defined in drupal_array_get_nested_value() (line 5748 of /var/www/html/drupalcvs/includes/common.inc).

chia’s picture

JoshOrndorff’s picture

Subscribe

JoshOrndorff’s picture

Subscribe

andypost’s picture

I file follow-up to discuss visibility of "wheel button" #961412: Formatter settings button lost when summary is empty
Comment #1 should be added to docs about current behavior

mlncn’s picture

andypost’s picture

Date module has troubles with current implementation #1023072: Customize default value - broken!

Date Field Settings uses fieldset elements to collapse relational data about default values, settings for the field are stored as nested array but when defaults are loading they goes plain array, maybe it's caused by #tree flag for fieldset is not working because Instance settings form is nested inside main fieldAPI form.

yched’s picture

@andypost : this doesn't sound like it's has anything to do with formatter settings and the 'display fields' screen ?

andypost’s picture

@yched I mean that we has inconsistency between actual form hierarchy and resulting values (which is a plain array)

yched’s picture

@andypost : your #193 mentions "default values" and "Instance settings form". Those have nothing to do with this thread, which was about the 'Manage display' screen.

  • Dries committed b135d64 on 8.3.x
    - Patch #796658 by yched, andypost: UI for field formatter settings.
    
    

  • Dries committed b135d64 on 8.3.x
    - Patch #796658 by yched, andypost: UI for field formatter settings.
    
    

  • Dries committed b135d64 on 8.4.x
    - Patch #796658 by yched, andypost: UI for field formatter settings.
    
    

  • Dries committed b135d64 on 8.4.x
    - Patch #796658 by yched, andypost: UI for field formatter settings.
    
    

  • Dries committed b135d64 on 9.1.x
    - Patch #796658 by yched, andypost: UI for field formatter settings.