Follow-up to #2831274: Bring Media entity module to core as Media module.
Problem/Motivation
In #2831274-200: Bring Media entity module to core as Media module, @effulgentsia commented:
This screenshot shows that when I add a new field somewhere (in this case to the Article content type), I'm presented a bunch of choices, andFile
andMedia
are two separate choices within the"Reference"
section. How do I know how these differ?
Over the course of much discussion, we realized that this is really a Field UI problem, not a Media problem. Even though Media is becoming the preferred, and eventually the default, way to handle files and images in Drupal, there are still valid use cases for good ol' file and image fields (and in fact, both File and Image modules are hard dependencies of Media) -- for instance, if you do not want to support metadata or re-use. So the real problem is how to make it obvious to users what the difference is between "File", "Image", and "Media" -- or, for that matter, between any of the similarly-named field types that one can create.
Proposed resolution
Split the problem in two phases:
1) Short-term goal (8.5 target)
Reduce confusion for inexperienced site-builders, while still allowing advanced users to use whatever they need, basically improving labels and adding more help (as suggested in #42 and #51).
Field type plugins already support descriptions, so those descriptions will be shown below the select list displayed when creating a new field. Only the description of the currently selected field type will be shown. Many of the current descriptions are very developer-centric, so they will be wordsmithed in the follow-up issue #2930446: [PP-1] Improve field description texts for fields provided by core.
This work will be done in this issue.
2) Long-term goal
Reach a consensus on:
- If file/image fields will eventually be necessary at all (when Media is the preferred solution in core). (#47, #31 )
- If they are necessary, how / when to present them on the UI, versus the "media" field ( #34, #37, #43 )
Remaining tasks
Commit the patch.
User interface changes
The "Add field" page (/admin/structure/{entity-type}/manage/{bundle}/fields/add-field
) will be extended to include new visual elements to help site builders understand the differences between the field types.
Screenshots with the the patch in #91 applied:
File field:
Image field:
Media field:
Video of the new descriptions being shown, with patch in #91 applied:
file_add_with_descriptions.mp4
API changes
None.
Data model changes
None.
Comment | File | Size | Author |
---|---|---|---|
#97 | interdiff-91-96.txt | 921 bytes | marcoscano |
#97 | 2862458-96.patch | 9.99 KB | marcoscano |
#94 | file_add_with_descriptions.mp4 | 444.53 KB | marcoscano |
#94 | file_description.png | 18.06 KB | marcoscano |
#94 | image_description.png | 18.5 KB | marcoscano |
Comments
Comment #2
rogerpfaffI wrote a patch to move the two options in the select list.
Comment #3
rogerpfaffComment #4
webflo CreditAttribution: webflo at UEBERBIT GmbH commentedIs image and file always present? I think this depends on the media type configuration.
This will fail on multilingual sites. field_ui_form_field_ui_field_storage_add_form_alter uses the t() function for the string.
Comment #5
rogerpfaffI have updated the patch to reflect multilingual option groups and added a check for the options before moving them.
Comment #6
rogerpfaffComment #9
rogerpfaffComment #10
rogerpfaffComment #11
marcoscanoI tested this manually and it works great, thanks @rogerpfaff! Just a couple of nitpicks:
This should be
Implements hook_form_FORM_ID_alter().
only. I know field_ui has it differently but it's wrong there too... :)This comment could be clearer IMHO. What about 'Move the "image" option to the "General" optgroup.', or something along those lines? (also, missing trailing dot :)
Same as before.
There should be an empty line at the end of the file.
Comment #12
dhruveshdtripathi CreditAttribution: dhruveshdtripathi at DevsAdda commentedAttached updated patch with changes suggested in comment #11
Comment #13
naveenvalecha@dhruveshdtripathi
Please provide the interdiff, it's really easy to know what has been changed. See how to create the interdiff https://www.drupal.org/documentation/git/interdiff
Attached is the interdiff.
Review of the interdiff:
Need to remove this extra line.
Comment #14
marcoscanoThis is moving the file option, not the image.
Comment #15
dhruveshdtripathi CreditAttribution: dhruveshdtripathi at DevsAdda commentedWorking on changes suggested in comment #13 and #14
Comment #16
dhruveshdtripathi CreditAttribution: dhruveshdtripathi at DevsAdda commentedHi @naveenvalecha
Removed extra line. Attached interdiff file this time.
Hi @marcoscano
Comment text changed according to your suggestion in comment #14
Comment #17
dhruveshdtripathi CreditAttribution: dhruveshdtripathi at DevsAdda commentedComment #18
dhruveshdtripathi CreditAttribution: dhruveshdtripathi at DevsAdda commentedComment #19
BerdirI don't quite understand why we don't just change the category annotation on those two field types? Why change the behavior only when media is enabled?
Comment #20
marcoscanoRe: #19, this was part of the outcome of a UX meeting review, so probably the best is to confirm with UX people if the idea behind this requirement is just when the media module is enabled or in general for core.
If it's a general issue though, I think we shouldn't keep this as a blocker for the media roadmap.
Comment #21
phenaproximaRe-tagging and adding the usability bat signal. We really need UX guidance on this one.
Comment #22
seanBI agree with Berdir in #19. Changing this conditionally is confusing. Attaching just a patch since an interdiff doesn't make any since.
Comment #23
Berdirunrelated settings change.
Comment #24
seanBThat was sloppy. Sorry about that!
Comment #25
Gábor Hojtsy@Berdir: I think the *goal* of this change is to encourage use of media instead and not confuse people with types of references to media-look-alikes that are not media. On the other hand, for people not using Media, I think the existing setup makes sense. So changing the category globally is applicable for sure in the future when Media is THE file/image solution, but until then it sounds like a regression (unless Media is enabled)? We definitely need to take a holistic look at this.
Comment #26
Gábor HojtsySo I think this is the problem being investigated here. If you want a file media or image media, do you pick file, image or media? :)
Our first inclination AFAIR was to just remove these two options, but media bundles do have image and file fields, so it needs to be kept for those. But of course once a site has media, we want to encourage use of media and not the bare-bones file and image that are not reusable.
OTOH moving the file and image options *up* in the list would probably not help as they would be even more apparently in your face to pick, so you may not reach media lower down the list... So not sure either patch in this issue is a UX improvement then. :/
Comment #27
marcoscanoWhat if, _only when media is enabled_, we create a new category at the very bottom called "-- Legacy --" (or "-- Obsolete --" / or whatever), and move file and image down there?
Comment #28
Gábor HojtsyUpdated title and issue summary based on comments from #2831274: Bring Media entity module to core as Media module.
Comment #29
Gábor HojtsyClosed #2862461: When adding a reference field, remove "File" from the offered entity types as a duplicate. Although not technically a 100% duplicate, that is the same problem one step later in the user flow. I think we should discuss in the same issue (agreed with @seanB from #2831274: Bring Media entity module to core as Media module, as copied to this issue summary).
Comment #30
Berdirnot sure about #2862461: When adding a reference field, remove "File" from the offered entity types being a duplicate, that is a different problem than this that is not relevant to media (the problems there are that file usage doesn't work, that there are no useful formatters and so on).
While it might like the same, File/Image pre-selected options below References are *not* the same as Content/Terms. Those are entity_reference + type preselected, while image/file are their own field type.
That's why I think moving to Generic makes always sense. For a user, file/image is *not* a reference. You don't reference something else, you just upload a file. that it technically is a field type that inherits from an entity reference and is a special case of a reference isn't really relevant to the user.
But, moving to generic doesn't actually address the problem of telling a user what to pick, Media or Image/File.
The problem is also that File/Image aren't actually legacy/deprecated nor will they be in the near future because the media entities themself still need those field types. I wrote more about that here: #2885002: Provide an optional migration path for sites using files/images to media entities.
Comment #31
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedI posted a comment in #2831943-125: Use "rendered media" (not links) as default media field formatter; add modal to configure the used media view mode that's very much related to the discussion here :)
TL;DR: When all the required parts for Media are in core (#2831943: Use "rendered media" (not links) as default media field formatter; add modal to configure the used media view mode, #2831940: Create file field widget on top of media entity, #2831941: [PP-1] Re-create Image field widget on top of media entity), I think we should completely hide the current FIle and Image options from the Reference group.
Comment #32
Gábor HojtsyWell, we would still not use media fields in the standard profile yet neither enable the module in that profile. So what would people using standard profile do? How do they create a new content type with an image field (similar to the content type they already get with an image field?).
If you meant we should hide these options completely if media is enabled then the question transforms into: how do you add a file or image field on a media entity if you need to? (Is there such a case where you need to do this other than the source fields already existing, I am not sure.)
Comment #33
marcoscanoThere certainly is. You could add a text file transcript to a video media entity, or in general any additional files / attachments to a media item. That's not uncommon I think.
Comment #34
Gábor HojtsyRight, so I think the general core goal is to avoid people being able to create File and Image fields on things other than Media items so they use media items instead, so we are trying to solve that.
Does moving File and Image to "General" from "Reference" has other benefits other than separating the types? (Which IMHO in itself does not help with the larger goal which is why I "fixed" the title and issue summary).
Comment #35
seanBAs far as I understood it, we move file / image until media is done. When media is ready to replace file/image, we hide file/image for every entity except media.
Comment #36
Gábor HojtsyHm, what does moving File/Image help with in the first pass? They would be up in the list and even more prominent, no? :)
Comment #37
Wim LeersThis sounds sensible to me. It means that we DO NOT HIDE "Reference -> Image" and "Reference -> File" for the form at
/admin/structure/media/manage/*/fields/add-field
, but we DO HIDE it everywhere else:/admin/structure/*/manage/*/fields/add-field
(i.e. for all entity types other thanMedia
).IOW: the UI only allows to add
image
orfile
fields tomedia
entity types.Comment #38
marcoscano#37: +1
Comment #39
seanBI believe that was part of an earlier UX review. Having file / image as entity references was confusing to some users. But maybe that's a separate issue.
And #37 was exactly how I believe it should be. The question that comes to mind is what should happen when you enable file/image but don't enable media? Should we maybe hide the file/image modules as well?
Comment #40
Gábor HojtsyYeah it does not seem to solve anything for media, in fact makes it less clear and easier to make mistakes, so I would not jump to that yet. :)
Comment #41
parijke CreditAttribution: parijke commentedAs talked about with SeanB. This issue has two major challenges:
Please give feedback on this solution
This patch only addresses the 1st challenge. We also created a test for it,
Comment #42
BerdirHm.
I don't think those use cases are limited to media entities. There are many reasons why you would want to have file or image fields with non-reusable files and then you don't need to use media.
According to https://twitter.com/gaborhojtsy/status/886995205899128833, most users likely or very likely need reusable media. But some don't, e.g. https://twitter.com/da_wehner/status/887027340928274437.
And those things are not mutually exclusive. Even if you use media for all your internal node types, you might have contact form file attachments or some public node forms when normal authenticated users can submit content. You probably don't want that stuff in your media library.
Private files are also not really thought through I think. Media entity currently have no view access limitations and private file access logic is not hierachic. Which means that media entities with private files will actually be public to anyone as long as the media entity is public. That's a complex problem to solve and until we do, => more use cases for file/image directly on other entities.
Maybe instead of removing those options completely, we should find a way to guide inexperienced users into chosing the option they probably want without preventing experienced users from doing what they need. As a simple alternative to removing, we could change the labels to something like "File (non-reusable)"? We have descriptions which would allow for better explanations but we afaik don't show them yet.
Comment #43
parijke CreditAttribution: parijke commentedPatches added for the second issue mentioned in #41
What is changed:
other entities: (nodes)
Media entities
Comment #44
Gábor Hojtsy@Berdir: re #42, excellent points for when files/images would still be useful/important to have
Comment #45
tacituseu CreditAttribution: tacituseu commentedIn non-publishing sites I often find the need for upload-once-and-forget File and Image fields for things like attaching documents related to given content (scans, case photos, manuals, correspondence - all of them of no use for other entities), there is an advantage to keeping those things simple.
Removing them only when Media is enabled is also poor choice as they both have their uses and can very well co-exist.
If it's confusing for the 80% use case so much then at the very least leave the option to add them to non-media content types under 'Reference\Other...'.
Comment #46
BerdirThat's the thing with Image/File and Other.... Selecting File under Other.. creates an entity reference field to a file entity which is useless and broken. That is IMHO the reason why having them in Reference is incorrect/confusing (not so much about fieldable or not, config entities and many other things are not fieldable and work just fine in there), because it is not a standard entity reference field.
I think there's another isue that wants to prevent File being an option in Other.... Image doesn't exist as it is not an entity type.
Comment #47
seanB#41 / #43 Thanks parijke! As discussed before I think we should go for the change in #43, but I guess #41 already helps.
#42
Being able to add a custom file/image field to media is still needed for at least the source field (although we could auto create that). The other fields could be done as references between media items.
I agree you don't need to use media if you don't re-use, but I think media has several advantages over traditional files. You actually need a separate experimental module to enable the media library, so re-use is not the only reason to use media over file/image.
The same argument could be made for the file listing. When you upload a file it is listed on admin/content/file as well. There could be different solutions to hide specific media entities from the library. The fact that media is fieldable already makes this easier for media than for files.
This is indeed a challenge that is still ahead of us!
I think that for the short term (8.4 / maybe 8.5), removing file/image might be too much. Improving UX / labels or whatever to help guide the user is the best we can do until we solve issues like the private files situation. For the long term though, I think the best solution is to have only 1 option (media) instead of 3 options. File and image functionality could be merged into media and after that those modules will be removed. This is probably a Drupal 9 situation, but imho that is what we should be aiming for. We have a million things to do before that is actually the case, but I hope we can agree that should be our end goal. We shouldn't rush hiding file/image, but I would prefer to visually remove them when we enable media in the UI.
Comment #48
tacituseu CreditAttribution: tacituseu commented@Berdir: I agree having it under 'Reference' is confusing, it feels like needlessly exposing implementation detail, and they should be, at least conceptually, under 'General'.
"File (non-reusable)" makes sense to me, or maybe something like "File (without library)".
Comment #50
Bojhan CreditAttribution: Bojhan as a volunteer and commentedI dont fully understand what to review here, it seems like we still have the issue and are just moving it around? That will just mitigate it slightly, not resolve it.
Comment #51
vijaycs85Though the title and issue summary sound like this is a very simple UI issue, it's not after reading 50 comments :) Here is my 2 cents (or penny/paises):
- Assuming having all 3 in field list is confusing non-technical user is wrong. It's quite confusing even for developer (probably not part of media initiatives) and #42 covers great variety of scenarios explaining difference between media and basic file system.
- Add more help on add field page. Since it has it's own page (instead of manage field in D7), we could provide more help text around files vs media or image vs media. This help content could also cover the long term plan (per #47)
- Probably this issue is not ready for UX review or won't be solving any UX issues.
Comment #52
Bojhan CreditAttribution: Bojhan as a volunteer and commentedPer @vijaycs85 his review, removing tag.
Comment #53
marcoscanoAfter going through all comments again, I believe we have two approaches here:
1) Short-term goal (probably 8.5.x ?)
Reduce confusion for inexperienced site-builders, while still allowing advanced users to use whatever they need, basically improving labels and adding more help (as suggested in #42 and #51 )
2) Long-term goal
Reach a consensus on:
- If file/image fields will eventually be necessary at all (when Media is the preferred solution in core). ( #47, #31 )
- If they are necessary, how / when to present them on the UI, versus the "media" field ( #34, #37, #43 )
- How to deal with the transition period between now and the moment where "Media is the preferred solution"
------------
I propose we focus on 1) here, and create a new issue only to discuss 2), which will probably take more time.
The attached patch tries to go on that direction, basically:
- Changing the category of "File" and "Image" to "[Non-reusable assets]" (in brackets to force them moving *down* the list, so being less prominent.)
- Showing the field descriptions on the form (for all fields that have descriptions on their annotations)
- Improving the descriptions of the "File" and "Image" field. (Wording suggestions would be very welcome)
Maybe we could also change "File" and "Image" labels to be even more explicit, but I thought that moving them to a new category and improving the description maybe was enough.
Once "Media" is not a field type, it has no description of its own, and inherits the EntityReference description. (which may be fine in this case?)
This is the result of the attached patch applied to 8.5.x HEAD:
(video) improve_help_add_field_storage.mp4
Note1: no tests were updated/added, if we agree on the approach I can work on that next.
Note2: no interdiff provided because this is a completely different patch.
Comment #54
BerdirI think I like that direction, feel free to remove the current descriptions with the integer and so on completely, we never used them so far and they are *clearly* way too technical :)
Comment #55
marcoscanoJust realized that maybe we shouldn't "downgrade" file/image categories if Media is not enabled, in the short term. This patch only changes those categories if Media is on.
Also, removed the previous file/image descriptions as suggested in #54, I agree they didn't add any value to the general site builder.
Thanks!
Without Media installed:
With Media installed:
Example of the description being shown (if you didn't see the video from #53):
Comment #56
xjmSwitching to the correct tag so the UX team sees this. :)
Comment #57
marcoscanoExtended the IS with the proposal from #53.
Comment #58
marcoscanoThis was discussed in Dec, 12th, 2017 UX meeting: https://www.youtube.com/watch?v=aWgHphVqzdk
The conclusions were:
- Showing the field descriptions is a good idea and we should move forward on that.
- We should make sure the description texts for File and Image are better than what they are now.
- We should investigate if it's feasible to make Media have its own description text (instead of inheriting the
entity_reference
description, as currently).These actions could be done in this issue, targeting to solve the confusion that appears when the Media module is enabled. The conclusion was that if good descriptions are available, there's no need to create new category groups or move options around the list, so this (the descriptions) should be enough to fix this issue.
As a follow-up, we should review the description texts of all core field types, checking if they make sense to a site builder audience, now that they are exposed on the UI.
Comment #59
yoroy CreditAttribution: yoroy at Roy Scholten commentedThanks for presenting the problem and possible solutions at the meeting & the correct summary @marcoscano :)
I like the approach of showing help text to clarify what each option is. It's one small sensible thing we can do without reshuffling items in the select list (breaking expectations) or overloading the content of the select list even more.
( As I mentioned on the call, this is probably the single most problematic dropdown in all of core. Completely out of scope here but very relevant to the problem at hand is this far more radical proposal: #2539740: Provide users with a visual way to understand and build content types (dream fields) as an experimental module. )
Comment #60
marcoscanoComment #61
marcoscanoIn this patch:
1) We are not messing with re-ordering / creating new categories anymore
2) Descriptions from plugin annotations are added to the form, for all fields
3) The
File
andImage
descriptions were improved4) A new hook is added (
hook_field_preconfigured_options_alter()
) to allowMedia
to have its own description, different from the other entity reference fields. (Note: this hook would be the same hook we are considering using in #2928699: Add an alter hook for the pre-configured field UI options and implement it in the Media module, so the new addition would benefit both issues).5) The hook is implemented for the
Media
option, and as an example, also for theContent
option. Entities that do not implement this hook will inherit the default entity reference field description.6) Added some tests.
Screenshots after this patch is applied:
No reordering / re-grouping:
File description:
Image description:
Media description:
Node description:
Other entities where the hook is not implemented:
Code and text/wording reviews will be very welcome.
Comment #62
marcoscanoAlso, created #2930446: [PP-1] Improve field description texts for fields provided by core as a follow-up to check all remaining descriptions.
Comment #63
phenaproximaI like where this is going.
This is already implemented by #2928699: Add an alter hook for the pre-configured field UI options and implement it in the Media module, so we should postpone this on that issue.
Two things. One, can we inject the plugin manager? And two, can we use getDefinition('entity_reference') instead of getDefinitions()?
Is this optional? If so, we should probably mention that...
I think this is supposed to be protected?
We shouldn't hard-code the description text in two places; IMHO, we should look up the actual text that we expect to appear, and assert the presence of that. Otherwise, the slightest change to the descriptions will break this test.
$field_definition['id'] will never be empty.
Comment #64
marcoscanoThanks @phenaproxima for reviewing!
Re-rolled on the top of #2928699-44: Add an alter hook for the pre-configured field UI options and implement it in the Media module, just to keep the ball rolling.
Also, addressed the feedback from #63.
Thanks!
Comment #67
phenaproximaThis looks great. Test looks great. I do, however, question the need for a MEDIA_FIELD_DESCRIPTION constant. Is there any way we can look that up in the test without needing to use a constant?
Comment #68
vijaycs85@phenaproxima was reviewing this issue today and was going to ask why do we need MEDIA_FIELD_DESCRIPTION :) then thought we needed just because we are using in both test and code. However it's very "special" case to have a const for description.
Comment #69
marcoscanoThis description string is special because it's already an override of the original
entity_reference
description. We do so by implementing the new alter hook:I can't come up with any better way of defining the string only once, and then retrieving it inside the alter hook and the test, as suggested in #63.5. If you have any suggestions on that, I'd be happy to change it in that direction!
IMHO though, perhaps just a plain string in both places should be enough in this particular case?
Thanks!
Comment #70
Berdir> I can't come up with any better way of defining the string only once
Then let's not, just write it out twice, agreed that this is fine. Arguments to t() *must* be plain strings, no constants, concatenation or other trickery allowed, otherwise it can not be parsed automatically.
So, I disagree with #65.3, this is standard procedure and changing the string is IMHO *supposed* to break the test.
Comment #71
marcoscanoThat makes sense, thanks!
Comment #72
phenaproximaOkay. I'm happy with it! The patch itself, as far as I'm concerned, is RTBC. Thank you, @marcoscano et. al!
I'm leaving this at NR since it needs a re-title, re-scope (this is squarely a Field UI issue and patch, not Media) and summary update.
Since this patch exposes what was previously a strictly developer-facing string (the field type description) in the UI, do we need a change record as well?
Comment #73
marcoscanoThanks @phenaproxima for the feedback!
I'm not 100% sure about the re-scoping. Media users are arguably the most affected ones by this confusion between "Image", "File" and "Media" fields (as shown on the screenshot in the IS), and we are indeed tweaking these 3 descriptions in the patch to improve specifically this use case. As a side effect, we are also exposing the description to all other fields, which will benefit other users of Field UI as well, but I wouldn't say that that's the main problem being solved here, IMHO. Not against changing it if you feel strongly about it, though.
On the other hand, if the patch looks close to ready, I think the best we can do now is just wait for #2928699: Add an alter hook for the pre-configured field UI options and implement it in the Media module to land.
Comment #74
BerdirThis also looks pretty good to me.
I'm also unsure about #2930446: [PP-1] Improve field description texts for fields provided by core being postponed on this. We are not showing it now, so doing it first is a bit strange, but the problem is that we expose *all* descriptions with this patch, not just those that we improved, and a number of those descriptions are currently really bad and might confuse users more than not having a description at all, so an argument can be made that committing this without the other issue will make usability worse than it is now.
While we can't really test it in the UI without this issue, it's also not technically blocked on this, so we could start improving the descriptions there and since this is still postponed (on an RTBC issue, so it will hopefully not take long), we have to wait a bit anyway?
Comment #75
marcoscanoYou are right! I too think we should try to improve the other strings in parallel as much as possible. For me they should ideally be committed at the same time, or that one shortly after this one. I have posted an initial suggestion at #2930446-3: [PP-1] Improve field description texts for fields provided by core, so that we can have something to start working with. Feedback would be very much appreciated.
Thanks!
Comment #76
benjifisherI am testing this, and when I looked at the displayed text for a Link field, it was formatted as a link. Here is the markup:
In the Bartik theme (and probably many others) there are CSS rules to make
class="link"
look like an<a>
tag.I am not sure there is a need to style each of these differently, so I am not sure we need to give each of them its own class. The simplest fix is to remove the class entirely.
If we do need to target these descriptions, then we should follow the guidelines in https://www.drupal.org/docs/develop/standards/css/css-architecture-for-d... and do something like this:
To be clear: my recommendation is to remove the class. If there is a real need for the CSS class, then do something like the above snippet.
P.S. I guess I meant the Seven theme, not Bartik.
Comment #77
marcoscanoThanks for spotting that @benjifisher!
Initially I was playing around with a way to have a unique identifier for the description container, but in the end the container itself is not necessary at all.
Thanks!
Comment #78
larowlanblocker is in
Comment #79
phenaproximaOkay, let's get this thing in. Re-uploading the no-dependencies patch from #77 to retrigger testbot and such.
Comment #80
marcoscanoRe-rolled and fixed the test I broke when the classes were removed in #77.
Comment #81
phenaproximaOnly two minor complaints, but otherwise this looks RTBC to me.
I don't see where this is being used?
We should use $assert_session->elementExists() instead of $page->find(). elementExists() returns the element itself, or fails the test if it doesn't exist. Better than running into a fatal error if $node_description isn't found! :)
Comment #82
marcoscano@phenaproxima thanks for reviewing!
Comment #83
phenaproximaLooks great. Thanks, @marcoscano!
All we need to do now is the administrative stuff (title and IS update), then this is RTBC.
Comment #85
phenaproximaFixing the title.
Comment #86
phenaproximaUpdated the IS.
Comment #87
phenaproximaChanged the IS again to better explain the scope of the problem.
Comment #88
marcoscanoThanks @phenaproxima for taking care of updating the IS and so on.
Marking as RTBC based on #83.
Comment #89
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedThe patch looks pretty good to me, I have only a couple of questions:
The
$name
variable should already be unique since it is the value selected in the option list, so I don't see why we need the extra$i
counter :)Why does this markup value have to live in a sub-container, can't we put it directly in
$form['description_wrapper']['description_{$name}']
?Comment #90
marcoscanoThanks for reviewing @amateescu!
1.
The reasoning was that strictly speaking, the
$name
here can be anything that modules define in their plugins and inFieldTypePluginManagerInterface::getPreconfiguredOptions()
implementations. If two modules implemented the latter with the same key, it could happen that it was not unique. In other words, a module could define$ui_definitions['General']['foo:bar:baz']
and another module$ui_definitions['Number']['foo:bar:baz']
. It is not such a problem in the select because if the categories are different, then the elements with the same key would be in different optgroups, but now that we are flattening all options together, I thought it would be safer just to add an extra unique counter to each one. But maybe it's just a too remote edge case and it's worth not doing it in favor of simplifying the code. Changed that.2.
That's the only way I could get the states working for the individual descriptions. If they are directly under the parent container, they all show up at once. Maybe I'm missing something obvious here, so please feel free to advise if you spot something that should be changed to make that work.
Thanks!
Comment #91
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedThat would be a much bigger problem than the description stuff that this patch is doing, because the field storage created by this form also relies on the fact that it is unique. So I agree that we don't need to worry about that here.
I was able to simplify the containers/states quite a bit and everything seems to be working as expected :)
I don't have any other complaints, so consider this patch as RTBC++ from me!
Comment #92
marcoscanoThank you @amateescu!
I have tested #91 manually, and it works great!
Am I allowed to RTBC it? :) (I'm doing it anyways, so if anyone thinks otherwise please just move it back.)
Thanks!
Comment #93
phenaproximaI discussed this on Slack with @larowlan. We need two things before this can land:
1) The IS needs to be updated with a screenshot (or, preferably, a video) demonstrating the change that this makes.
2) We'd ideally like @yoroy (or another usability expert) to approve the wording we came up with. Although we can always change it later, in #2930446: [PP-1] Improve field description texts for fields provided by core.
Comment #94
marcoscanoAdded screenshots and video to the IS showing the final descriptions.
Comment #95
xjmHm, I'm not sure that the fix proposed here is sufficient to address the issue. In general, people don't really read help text. Also, the text is on the page after the page where the user needs to make the choice.
This patch could help, but I'm not sure it's sufficient to resolve the UX issue with field selection so we can show the module in the UI. Are there other followups related to these first two screens?
On the patch itself, the descriptions are a bit wordy and technical, so it'd be good to simplify the language.
We might also want to create a separate issue that adds
hook_help()
info on file fields vs. image fields vs. media fields (and how image and file are used internally by media). Then we could link that page in descriptions like these when it comes.Comment #96
xjmDiscussed with @phenaproxima. While the work on descriptions will help users make a better choice, I don't think that change alone is sufficient to resolve the overall problem space here. So, let's rescope this back to the problem space explored by all the discussion to date, and then create child issues. @phenaproxima will spin off a separate issue to allow Field UI to expose the descriptions, and then another separate to adapt the descriptions.
Thanks!
Comment #97
marcoscano@Gábor Hotjsy provided some feedback on slack:
This patch addresses the typo.
About the extra margins, they are indeed there, and the reason is that if the descriptions are a form item, they get the
.form-item
class, which adds 0.75em of margin to them. Maybe someone more front-end savvy could advise on the best approach to avoid that, while still keeping the states fixed in #91 working.Comment #98
benjifisherI adapted the patch from #97 and submitted it to #2934860: Show field type descriptions when creating a new field. Since that issue does not change any of the descriptions, I guess the change between #91 and #97 did not make any difference after I was done with it.
Comment #99
benjifisher@joachim raises some good points in #2930446: [PP-1] Improve field description texts for fields provided by core (Comments #10 and #13). I think this issue is the right place to continue the discussion.
In #2930446-11, @marcoscano says that these descriptions are not used anywhere else. Are we sure of that? For the sake of discussion, I will assume that it is true.
In #2930446-10, @joachim says, "You can't, for example, display them somewhere on an existing field." Could you be more specific about where this text could be used? Are you thinking of a node-edit form, for example?
I think there is general agreement that we need better help text on the "Add field" page (
/admin/structure/types/manage/*/fields/add-field
). This was discussed at the Dec. 12th, 2017 UX meeting. See comments above.I also think it is clear that the text should be provided by the the FieldType plugins, or at least by the modules that provide these plugins.
So I think we have three options:
I think that (1) is the most convenient in the short run. It also seems like the most frugal, sort of like recycling.
I should also point out that none of these options handles the case of FieldType plugins that have variants, such as the core EntityReferenceItem. The last patches on this issue before it was converted to a meta used
getPreconfiguredOptions()
and the recently addedhook_field_ui_preconfigured_options_alter()
to supply the text we need for this issue. If anyone can suggest a fourth option that handles the various types of entity reference in the same way as other FieldType plugins, then I will drop my support for (1)!Option (1) is the approach currently being used in the first three child issues (#2930446, #2934881, #2934860). The fourth child issue (#2934885) is independent.
Comment #100
joachim CreditAttribution: joachim commented> @marcoscano says that these descriptions are not used anywhere else. Are we sure of that? For the sake of discussion, I will assume that it is true.
Note that that's true of core. We don't know whether anything in contrib uses these.
> In #2930446-10, @joachim says, "You can't, for example, display them somewhere on an existing field." Could you be more specific about where this text could be used? Are you thinking of a node-edit form, for example?
Two places we might use this spring to mind:
- the field edit form
- the field list for a bundle
Once #2539740: Provide users with a visual way to understand and build content types (dream fields) as an experimental module. changes the UI for adding fields, it's feasible we'd want to similarly improve the UI for listing and editing existing fields, and we'd want to use description text there.
Comment #101
xjmThanks @benjifisher for the thoughts! There is another option we discussed, which is simply adding it to the help block on the page. It might be a bit tricky or need a dash of JS to make the text change based on the selection/form submission, but it would be a way to solve our specific need without needing to add API surface. We could then change it later in followups if we do end up doing work on the descriptions.
I also want to second this from @joachim (I raised the same thing in our discussion IRL yesterday):
I think that going forward it would be good to keep the discussion about system-wide changes to the Field UI in #2934860: Show field type descriptions when creating a new field. We can probably copy the key parts of the discussion over there. I don't want to creep the scope of the initial change. I'll ask someone to work on that today.
Comment #102
xjmOh also, note that we have this additional child issue:
#2934885: Document the difference between File, Image, and Media fields
I see that the current direction of using descriptions was changed back in #58. I'm wondering though if consideration was given to the fact that people often don't read help text? Could we consider also changing the labels, reordering, etc.? Could have other child issues.
Comment #105
webchickLogging some additional issues that @marcoscano brought up during the 8.7/8.8 roadmap discussion:
Comment #111
rootworkThis was so close 3 years ago. I'm wondering if there's still interest in this issue, especially since it's postponing the even more important #2930446: [PP-1] Improve field description texts for fields provided by core (which also hasn't had progress in 3 years).
While Dream Fields is probably the ultimate destination (#2539740: Provide users with a visual way to understand and build content types (dream fields) as an experimental module., also no progress in 3 years) addressing the field labels and descriptions would go a long way to improving site-builder experience (SBX?) in my opinion.
Comment #116
Martijn de WitDoes the #3346539: [Plan] Improve field creation experience fix this issue concerns?