(This is the result of a discussion we had at DrupalCon Portland.)
Views' "disable field default class" option strips all markup and hence breaks in-place editing. Apparently this is a commonly used feature, and would thus make in-place editing within views appear very broken wherever it is used.
There is a solution to that: "lifting" of data- attributes to the "cell" (depending on the style plugin (format): <td>
, <li>
…). Then Edit will have sufficient metadata to still allow for for in-place editing as long as fields are not merged (in that case, lifting should only happen for the first field?).
Related issues
This issue blocks #1962606: Quick Edit + "Field" Views (table, grid …).
Comments
Comment #1
xjmComment #2
dawehneroh.
Comment #3
damiankloip CreditAttribution: damiankloip commented@dawehner, you sound excited...
Comment #4
dawehnerWell, VDC wasn't in the tag list before comment #2
Comment #5
xjmThis is kind of a big deal.
The title said:
Shouldn't we just do it always? Having it mysteriously flip on and off would be bad DX, I think.
Comment #6
dawehnerWhen "disable field default class" is not enabled, there is a field template file used instead, and based on that quickedit has everything which is needed already.
In general we have to think about the consequences for themers, because they currently really enjoy that views is producing the markup they have configured. An additional div will cause issues, for example with the inline functionality in views.
Comment #7
xjmI don't think anyone said anything about adding another div? I'm confused. It's about putting the attribute on the
<td>
,<li>
, etc.Comment #8
dawehnerI was answering your suggestion:
PS: This post was certainly not sent from the university workspace
Comment #9
xjmHehe. Sorry, I meant always add the attribute when there is a semantic container element. So this:
And this:
Not:
If someone is using an unformatted list and not adding the default markup, they're probably on their own.
Comment #10
dawehnerMade some progress on working this tonight.
Comment #11
dawehnerBeside from general feedback it would be cool if someone could have a look at the first hunk and suggest what should be the data which is added by default ...
Comment #12
dawehnerReview-bump ...
Comment #13
Wim LeersSorry for the delay.
For starters, here is a reroll.
Comment #14
Wim LeersGeneral patch perspective
As the issue indicates, the ideal would be that each view cell that contains a Field API Field automatically lifted any
data-
attributes added byhook_preprocess_field()
implementations into the view cell's attributes.If that can't happen, then modules like Edit need some way of 1) introspecting view cells' contents, 2) depending on those contents, set view cell attributes.
The current patch reviewed
The patch in #12 doesn't implement this automatic lifting of
data-
attributes. But how could it? After all,hook_preprocess_field()
is not called at all — because this is "the Views render pipeline", not "the Entity/Field render pipeline".So, consequently, the only way we could get the required attributes on each view cell, is by providing the ability to introspect each view cell's contents and then set attributes on the view cell. (In the case of Edit: essentially duplicate Edit's
hook_preprocess_field()
implementation to work with View cells.)I tried to do this, and surely it is possible:
Yay!
But this of course MAY NOT be hardcoded in
Drupal\field\Plugin\views\field\Field
. Modules should get the opportunity to apply logic such as that above. In other words: we need a hook.Now, this is nigh identical to what I did for Edit module in #1962606-15: Quick Edit + "Field" Views (table, grid …) over 4 months ago: implement
hook_preprocess_views_view_field()
, where I calculated thedata-edit-id
attribute and set it:The only difference is that I now need to be able to set this "view cell attribute". The code back then did not yet have the ability to set the attribute, the patch in this issue does not yet have the ability to let other modules run it. But is it really worthwhile to fire another hook? Wouldn't it be simpler to just use
hook_preprocess_views_view_fields()
, but allow$variables['attributes']
to be set?I don't know the Views architecture well enough to answer that.
To move forward
We have two options:
getCellAttributes()
via a hook.hook_preprocess_views_view_field()
to set$variables['attributes']
.Once we have either, in-place editing of Views will become as feasible as it ever will be.
Comment #15
Wim LeersA sister issue to enable in-place editing of Views in D8 contrib on the Edit side of things: #2083387: Make it possible to use in-place editing on entities rendered by a custom render pipeline (e.g. Views).
Comment #16
dawehnerWell, sure, we could somehow call the preprocess functions manually to get this information out. I don't think this will be a problem in terms of performance though we might should be somehow able to disable that behavior. (A config option without any UI could work)
So yeah my idea was that a generic fieldapi field could set enough information so that edit (and other modules) can deal with it and don't need anything more. It seemed for me that entity-type entity-id field-name combination and maybe field-instance-id etc. would be enough and you don't need anything more. (no hook / other preprocess). From your comments this sadly doesn't seem to be the case. It would be great
to get some feedback whether you need a specific combination so that a generic solution can't work.
From an API point of view css classes are part of the render api of views, so bypassing that in preprocess functions feels wrong. We do already have functions for particular elements, so that I went with the concept of cell classes/attributes. As properties aren't fieldapi fields I am wondering how we can also support them with a generic data pattern.
Luckily there are just two data points available yet, so the next review-break could also last just a short time.
Comment #17
Wim LeersThat should indeed be sufficient metadata, as long as you also include view mode and langcode.
That's not what I said. What I said was that Views' code can't hardcode setting the
data-edit-id
attribute for Edit module. Hence Edit module needs some way — a hook, a callback, something — to take that metadata you mentioned earlier (see the above quote) and then turn it into the attribute that is desired.So I think you might have misinterpreted my comment? (Or my comment just might be not sufficiently clear, of course.)
Comment #18
dawehnerYeah I though a generic attribute would be enough while you think that the metadata itself are needed in order to provide a specific data attribute.
I am fine with introducing a hook. Maybe we should add this hook once for each results.
Comment #19
Wim LeersStill tracking this, but since we can't push this forward, not really part of the Spark sprint. The edit.module-side of this problem is part of this sprint because it's not blocked (by this or any other issue), we're working on that in #2083387: Make it possible to use in-place editing on entities rendered by a custom render pipeline (e.g. Views).
Comment #20
Wim LeersBooooo, d.o tag monster!
Comment #21
Wim LeersThird time's a charm!
(I hope.)
Comment #21.0
Wim LeersUpdated issue summary.
Comment #22
Wim LeersJust FYI: the necessary API changes to make in-place editing of Views possible in Edit have landed in #2083387: Make it possible to use in-place editing on entities rendered by a custom render pipeline (e.g. Views). The only remaining blocker are the necessary API changes in Views, i.e.: this issue.
Comment #23
xjm13: vdc-2010946-13.patch queued for re-testing.
Comment #25
Renee S CreditAttribution: Renee S commentedAll the other related issues have been bumped to 8.1, so, moving this up also...
Comment #41
smustgrave CreditAttribution: smustgrave at Mobomo commentedBrought this up in #yes-no-queue and agreed with @larowlan this can probably be closed out with inactivity.
If still a valid feature request please reopen updating issue summary for how it pertains to D10
Thanks!