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.
Comment | File | Size | Author |
---|---|---|---|
#184 | field_ui-display-settings-d7.png | 32.34 KB | andypost |
#163 | field_ui_formatter_settings-796658-163.demo_.patch.txt | 24.71 KB | yched |
#163 | field_ui_formatter_settings-796658-163.patch | 19.06 KB | yched |
#148 | field_ui_formatter_settings-796658-136-demo.patch.txt | 30.37 KB | yched |
#148 | field_ui_formatter_settings-796658-139.patch | 24.38 KB | yched |
Comments
Comment #1
yoroy CreditAttribution: yoroy commentedMy 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…
Comment #2
joachim CreditAttribution: joachim commentedThere 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:
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.
Comment #3
yched CreditAttribution: yched commented"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.
Comment #4
yched CreditAttribution: yched commentedChallenges :
- 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...
Comment #5
joachim CreditAttribution: joachim commentedI 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?
Comment #6
yched CreditAttribution: yched commented"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)
Comment #7
joachim CreditAttribution: joachim commentedhow about using ahah like on http://drupalcode.org/viewvc/drupal/contributions/modules/examples/ahah_...
Comment #8
yched CreditAttribution: yched commentedRetitle.
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.
Comment #9
yched CreditAttribution: yched commentedInitial 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.
Comment #11
yched CreditAttribution: yched commented#9: field_ui_formatter_settings-796658.patch queued for re-testing.
Comment #12
yched CreditAttribution: yched commentedforgot to add : this is of course ajax-driven, and degrades nicely to a multistep form.
Comment #14
andypostHere 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 loopComment #16
andypostFrom 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
Comment #17
andypostPatch for win line-ending posted as follow-up #553298-160: Redesign the 'Manage Display' screen
So this patch without this change
Comment #19
yched CreditAttribution: yched commentedwindows 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.
Comment #20
yched CreditAttribution: yched commentedNew 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.
Comment #22
Bojhan CreditAttribution: Bojhan commentedWhy 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.
Comment #23
andypostLooking 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.
Comment #24
aspilicious CreditAttribution: aspilicious commentedThe message is needed as not everyone knows what the yellow color indicates.
Comment #25
andypostAnother idea - make settings collapsed under link like l10_server does.
Button "Settings" could be changed to ajaxed text-link "Change options"
Comment #26
andypostAnother round...
Suppose Delete could be changed to "reset to defaults"
Comment #27
yched CreditAttribution: yched commentedre 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.
Comment #28
yched CreditAttribution: yched commentedOops. Obviously some copy/paste went terribly wrong. Setting back title.
Comment #29
yched CreditAttribution: yched commentedUpdated 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)
Comment #31
yched CreditAttribution: yched commentedFWIW, 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.
Comment #32
yched CreditAttribution: yched commentedfiled a bug at #809292: FAPI #type image_button broken[edit: forget it, I'm a jerk]Comment #33
yched CreditAttribution: yched commentedHere's a working patch with an 'image_button' (screenshot in #31 above)
Comment #35
joachim CreditAttribution: joachim commentedI 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
Comment #36
aspilicious CreditAttribution: aspilicious commented#33: field_ui_formatter_settings-796658-33.patch queued for re-testing.
Comment #37
yched CreditAttribution: yched commentedre 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
Comment #38
yched CreditAttribution: yched commentedUpdated shot for the 3rd step
Summary of the screenshots :
step 1
step 2
step 3
Comment #39
tstoecklerIn 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.
Comment #40
yched CreditAttribution: yched commentedNo 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
Comment #41
Bojhan CreditAttribution: Bojhan commented@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.
Comment #42
tstoeckleryched in #37:
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.
Comment #43
yched CreditAttribution: yched commented[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
- for module config, the 'configure' link itself, indicating that this will take you to another page.
--> proposals :
- a more 'buttony' version :
- something like the 'dropdown arrow' used for blocks - with the 'unfolded' button acting like 'Cancel' ?
Comment #44
Bojhan CreditAttribution: Bojhan commented@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.
Comment #45
amc CreditAttribution: amc commentedThe 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.
Comment #46
yched CreditAttribution: yched commentedre @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.
Comment #47
joachim CreditAttribution: joachim commented> 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.
Comment #48
Bojhan CreditAttribution: Bojhan commentedSo 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.
Comment #49
joachim CreditAttribution: joachim commented> 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... :/
Comment #50
yched CreditAttribution: yched commentedExamples 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.
LOL :-)
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.
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 ?
Comment #51
amc CreditAttribution: amc commentedI 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.
Comment #52
Bojhan CreditAttribution: Bojhan commentedAl 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.
Comment #53
tstoecklerWhile 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.
Comment #54
yched CreditAttribution: yched commentedThanks 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.
Comment #55
Jeff Burnz CreditAttribution: Jeff Burnz commentedAside 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.
Comment #56
aspilicious CreditAttribution: aspilicious commentedFoo 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.
Comment #57
yched CreditAttribution: yched commented@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.
Comment #58
yched CreditAttribution: yched commentedSo, 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 :
alternate proposal 1:
alternate proposal 2:
- 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.
Comment #59
Jeff Burnz CreditAttribution: Jeff Burnz commented@56, 57, thanks people, good explanations.
Comment #60
moshe weitzman CreditAttribution: moshe weitzman commentedI 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.
typo?
Comment #61
yched CreditAttribution: yched commentedYes, 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.
Comment #62
yoroy CreditAttribution: yoroy commentedI'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
Comment #63
yched CreditAttribution: yched commentedre #62 :
Odd. Can you make sure you empty your sites caches ?
Is that with a default install, or are there contrib modules enabled ?
Comment #64
yched CreditAttribution: yched commentedYoroy : 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)
Comment #65
yoroy CreditAttribution: yoroy commentedHmm, apologies, can't reproduce. Installed a fresh head, no more errors.
Comment #66
yched CreditAttribution: yched commentedRight, that's probably because #553298: Redesign the 'Manage Display' screen requires a fresh install.
Comment #67
yched CreditAttribution: yched commentedThat 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...
Comment #68
aspilicious CreditAttribution: aspilicious commentedFor 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).
Comment #69
joachim CreditAttribution: joachim commentedHaving 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?
Comment #70
joachim CreditAttribution: joachim commentedPS. 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
Comment #71
yched CreditAttribution: yched commentedre #70: That's the same as Yoroy's #62. Did you reinstall HEAD after #553298: Redesign the 'Manage Display' screen got in ?
Comment #72
joachim CreditAttribution: joachim commentedI hadn't, but now I have, and yup, it was the same as #62: all fine now.
Comment #73
yoroy CreditAttribution: yoroy commentedMaybe something like this; expand and show the setting forms *below* instead of on the right side:
Comment #74
joachim CreditAttribution: joachim commentedThat 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.
Comment #75
aspilicious CreditAttribution: aspilicious commentedHow many settings can a field handle?
What happens with the UI when there are multiple settings?
Comment #76
yoroy CreditAttribution: yoroy commentedjoachim: 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? :)
Comment #77
yched CreditAttribution: yched commentedSee #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)
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.
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.
Comment #78
yched CreditAttribution: yched commentedUpdated 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.
Comment #79
yoroy CreditAttribution: yoroy commented'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.
Comment #80
yched CreditAttribution: yched commentedre #79 - makes sense.
"it forces you to go back and forth with your mouse between left and right" is still true, though ?
Comment #81
yoroy CreditAttribution: yoroy commentedI would aim to make as much of the table cell as possible clickable for config, not just the gear alone.
Comment #82
yched CreditAttribution: yched commentedNot 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.
Comment #83
yched CreditAttribution: yched commentedeasier tracking
Comment #84
yoroy CreditAttribution: yoroy commentedquicky feedback :) http://skitch.com/yoroy/dgnri/formatter-settings
Comment #85
yoroy CreditAttribution: yoroy commentedComment #86
aspilicious CreditAttribution: aspilicious commented1) 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?
Comment #87
yched CreditAttribution: yched commentedre #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
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.
Comment #88
yched CreditAttribution: yched commentedOK, 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)
Comment #89
amc CreditAttribution: amc commentedIf 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.
Comment #90
yched CreditAttribution: yched commentedSo here's a mockup using the pen icon from yoroy's http://drupal.org/project/protocons :
Comment #91
yched CreditAttribution: yched commentedbuttony version :
Comment #92
yched CreditAttribution: yched commentedAdjusted 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 ?
Comment #93
yoroy CreditAttribution: yoroy commented- 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.
Comment #94
yoroy CreditAttribution: yoroy commentedAnyway :)
Variations on form placement, button placement and how the summary could be presented.
Comment #95
yched CreditAttribution: yched commented[edit : oops, crosspost with #94]
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.
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).
Comment #96
yched CreditAttribution: yched commentedre #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...
Comment #97
yoroy CreditAttribution: yoroy commentedAnd I don't even like sugar in my coffee! :) I too would love some reviews from others.
Comment #98
yched CreditAttribution: yched commentedInvestigated 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.
Comment #99
Bojhan CreditAttribution: Bojhan commentedInteresting, 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.
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.
Comment #100
yched CreditAttribution: yched commentedre #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.
Comment #101
yoroy CreditAttribution: yoroy commentedyeah, 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!
Comment #102
amc CreditAttribution: amc commentedRegarding 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.
Comment #103
yched CreditAttribution: yched commentedUpdated patch and screenshot.
Comment #104
andypostI 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!
Comment #105
yched CreditAttribution: yched commentedre #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
Comment #106
yoroy CreditAttribution: yoroy commentedWe'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 :)
Comment #107
moshe weitzman CreditAttribution: moshe weitzman commentedI 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.
Comment #108
Bojhan CreditAttribution: Bojhan commented@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.
Comment #109
yched CreditAttribution: yched commented[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
Comment #110
joachim CreditAttribution: joachim commented> 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.
Comment #111
yched CreditAttribution: yched commentedIt'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 :-)
Comment #112
Bojhan CreditAttribution: Bojhan commented@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.
Comment #113
yoroy CreditAttribution: yoroy commentedI'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):
Comment #114
yched CreditAttribution: yched commentedPatch 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.
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.
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.
Comment #115
yoroy CreditAttribution: yoroy commentedOr stay in edit mode: yes. lets you explore options. Going back to summary mode on changing the dropdown is confusing.
Comment #116
Bojhan CreditAttribution: Bojhan commented@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.
Comment #117
andypostAs for me it's more intuitive #114. And Bojhan give a good explanation why!
Comment #118
yoroy CreditAttribution: yoroy commentedThanks for that explanation bojhan, agreed that hiding the dropdown is the better option.
Comment #119
joachim CreditAttribution: joachim commentedI 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.
Comment #120
yoroy CreditAttribution: yoroy commentedYeah, 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.
Comment #121
yched CreditAttribution: yched commentedFrom my #114 on the 'remove the select' version (screenshot)
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.
Comment #122
yched CreditAttribution: yched commentedOops, 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.
Comment #123
aspilicious CreditAttribution: aspilicious commentedFor me it looks great :)
Comment #124
yoroy CreditAttribution: yoroy commentedyep, go with a styling that matches a form label (bold)
Comment #125
Bojhan CreditAttribution: Bojhan commentedYup, good!
Comment #126
mfbSubscribe. I'm upgrading a CCK field module and this works very nicely.
Comment #127
yched CreditAttribution: yched commentedUpdated 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.
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 ?
Comment #128
yched CreditAttribution: yched commentedFixed 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.
Comment #129
yched CreditAttribution: yched commentedFixes a minor vertical misalignment for updated summaries.
Comment #130
aspilicious CreditAttribution: aspilicious commentedthe 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.
Comment #131
yched CreditAttribution: yched commented#130 : true, but that's a bug in ajax.js, not for this thread to fix.
Comment #132
yched CreditAttribution: yched commentedShameless 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
Comment #133
yched CreditAttribution: yched commentedCleaner 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.
Comment #134
yched CreditAttribution: yched commentedanyone ?
Comment #135
joachim CreditAttribution: joachim commentedOverall 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?
Comment #136
yched CreditAttribution: yched commentedajax.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.
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() :-(.
It works as if you clicked 'update' first. You can be lazy :-).
Comment #137
moshe weitzman CreditAttribution: moshe weitzman commentedI'm ready to RTBC this as yched has replied to all concerns. But it looks like example code is in the image formatters.
Comment #138
moshe weitzman CreditAttribution: moshe weitzman commentedOn second thought, the CVS committers might appreciate that example code so lets send it to them as is.
Comment #139
yched CreditAttribution: yched commentedPatch 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)
Comment #141
amc CreditAttribution: amc commented#139: field_ui_formatter_settings-796658-139.patch queued for re-testing.
Comment #143
tstoeckler#139: field_ui_formatter_settings-796658-139.patch queued for re-testing.
Comment #144
yched CreditAttribution: yched commentedTestbot hiccup. Now green, back to RTBC. Thks for the retests, folks :-).
Comment #145
yoroy CreditAttribution: yoroy commentedTesting 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.
Comment #146
yched CreditAttribution: yched commentedYoroy : 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.
Comment #147
yoroy CreditAttribution: yoroy commentedDuh, sorry :)
Comment #148
yched CreditAttribution: yched commentedCool :-).
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.
Comment #149
yched CreditAttribution: yched commentedbump, + the usability review was done :-)
Comment #150
Bojhan CreditAttribution: Bojhan commented/me disables
Comment #151
Manuel Garcia CreditAttribution: Manuel Garcia commentedsorry for subscribing but well, I'm planing on porting galleryformatter to d7 and this is critical for it.
Comment #152
andypostGallery API also waiting this patch to be commited.
Comment #153
sunWhew, quite a patch. Would love to see technical in-depth JS and AJAX reviews and/or approvals here.
Comment #154
Manuel Garcia CreditAttribution: Manuel Garcia commentedI'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.
Comment #155
yched CreditAttribution: yched commentedre #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 :-).
Comment #156
yched CreditAttribution: yched commentedAlso, 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.
Comment #157
EvanDonovan CreditAttribution: EvanDonovan commentedSubscribing to get back to, but don't know whether my level of knowledge will be helpful. Would love to see these settings though.
Comment #158
Crell CreditAttribution: Crell commentedSubscribe. 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!
Comment #159
bleen CreditAttribution: bleen commentedRTBC +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
Comment #160
Manuel Garcia CreditAttribution: Manuel Garcia commentedIf 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/
Comment #161
sunThis 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.
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.
1) Missing JSDocBlock.
2) Missing closing/trailing semicolon after function declaration.
AFAICS, the #op of the select list should actually be 'change_type'.
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?
Both hooks should get $form and most importantly $form_state passed.
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.
(minor) Duplicate + missing space here.
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!
Comment #162
yched CreditAttribution: yched commentedBasically, 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.
Comment #163
yched CreditAttribution: yched commented#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.
Comment #164
moshe weitzman CreditAttribution: moshe weitzman commentedAnd once again, we get to RTBC. Quick, lets do this.
Comment #165
yched CreditAttribution: yched commentedsad panda bumps...
Comment #166
mcreature CreditAttribution: mcreature commentedSubscribing
Comment #167
moshe weitzman CreditAttribution: moshe weitzman commented#163: field_ui_formatter_settings-796658-163.patch queued for re-testing.
Comment #168
Manuel Garcia CreditAttribution: Manuel Garcia commentedJust 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! =))
Comment #169
yoroy CreditAttribution: yoroy commentedThanks 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'.
Comment #170
yched CreditAttribution: yched commented@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.
Comment #171
Manuel Garcia CreditAttribution: Manuel Garcia commentedOK, 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!
Comment #172
sunbump; we get a deferred dependency here, which is ideally finished before beta, too.
Comment #173
loominade CreditAttribution: loominade commentedsubscribing
Comment #174
sunI am elevating this patch to critical. Everyone agrees.
Comment #175
yched CreditAttribution: yched commentedThis 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...
Comment #176
Dries CreditAttribution: Dries commentedFinally 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!
Comment #177
Dries CreditAttribution: Dries commentedComment #178
andypostLet's document this, #127
Comment #179
joachim CreditAttribution: joachim commentedIronically, 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 ;)
Comment #180
yched CreditAttribution: yched commented@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 !
Comment #181
andypost@yched I mean to drop a line into http://drupal.org/node/224333
Comment #182
yched CreditAttribution: yched commentedre @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.
Comment #183
EvanDonovan CreditAttribution: EvanDonovan commentedSeems 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?
Comment #184
andypostSomething strange with button for changing formatter settings for field
EDIT: Related Issue #883336: theme_image_button() broken (needs tests only)
Comment #185
yched CreditAttribution: yched commentedYeah, I got it in french on my own setup.
I think theme_image_button() got broken in #690980: Disabled form elements not properly rendered
Comment #186
BenK CreditAttribution: BenK commentedTracking this thread...
Comment #187
chia CreditAttribution: chia commentedSomething 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).
Comment #188
chia CreditAttribution: chia commented#896162: Write tests for the Field UI formatter settings form fixed the above issue
Comment #189
JoshOrndorff CreditAttribution: JoshOrndorff commentedSubscribe
Comment #190
JoshOrndorff CreditAttribution: JoshOrndorff commentedSubscribe
Comment #191
andypostI 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
Comment #192
mlncn CreditAttribution: mlncn commentedUser expectation fail with the current implementation: #1025976: Updating a field formatter at Manage display fields does not save the change, and the user is not warned of this
Comment #193
andypostDate 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.
Comment #194
yched CreditAttribution: yched commented@andypost : this doesn't sound like it's has anything to do with formatter settings and the 'display fields' screen ?
Comment #195
andypost@yched I mean that we has inconsistency between actual form hierarchy and resulting values (which is a plain array)
Comment #196
yched CreditAttribution: yched commented@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.