Give display modes a description

Nice suggestion! This could be one potential way to try to address #2844203: Improve/Simplify situation around Default/Full view modes/view displays. 👍

The description field is added, but I was not able to edit and save the description of an existing display mode, nor was I able to save the description that I added while creating the display mode as seen in the screen recording attached below. Changing the status to 'Needs work' for now.

Updated the entity keys. Please review as it resolved the issue mentioned in #15.

With updates to existing config it'll need an upgrade path.

Because all of the new values are '' or null, it shouldn't be needed. But an empty post_update hook could be added to force a cache clear

My question is that why are half of the new values '' and the other half are null?

Code all looks good, and most of the changes are just updating schemas to have an empty string for description:. I also tested locally and added some descriptionless view modes that weren't part of the default install, and running update.php after switching to this branch added description support gracefully.

If this feature is actually useful should we provide some useful defaults, at least for new installs? Umami is also supposed to showcase best practices so we should add some descriptions there too I think.

Re #22 I'd like to make sure the description language gets the attention it deserves while not obstructing the addition of this functionality, so I created a followup to this for writing the actual descriptions #3371297: Compose view mode descriptions. There are also funded contributors able to work on it as part of Field UX efforts, so this wouldn't be a throwaway.

I'm setting it back to RTBC since that work is part of the queue, but if there's disagreement with this take I'm amenable.

+1 that it should be fine to add the view mode descriptions in a follow-up.

Haven't looked at the code in detail but I'm wondering if we should show the description in the Manage form display and Manage display pages too?

I'm wondering if we should show the description in the Manage form display and Manage display pages too

Yeah that crossed my mind too. Since we know there's contributors tacking Field UX issues I'd be fine with that as another followup especially since it's a clean way to break the work down and keep the MRs smaller.

+1 to this.

Agree with you that we'd need to show this on:

• The view mode list, same way as we have descriptions on node lists
• The view mode form on the entity Manage Display/Manage Form Display

Something similar to what Display Mode Guidelines is doing, but it'll need some work to create the mocks/come up with a visual solution to know where to place it and how it needs to look like, so a follow-up to discuss it would be great and we don't block this work.

Added few comments to the MR. 👍

Created #3371389: Display view option descriptions in the Manage form display and Manage display pages for adding these to manage form display and manage form. Scoping it to its own issue will ensure we don't cut corners on the design choices.

Hmm, tests are failing because the typehint states that the property must be a string. This is not the case when a new display mode is being created because the description isn't set yet. We could either not use \Drupal\Core\Entity\EntityDisplayModeInterface::getDescription there or we could allow null values:

diff --git a/core/lib/Drupal/Core/Entity/EntityDisplayModeBase.php b/core/lib/Drupal/Core/Entity/EntityDisplayModeBase.php
index 1aaf1687c5..ff368f258e 100644
--- a/core/lib/Drupal/Core/Entity/EntityDisplayModeBase.php
+++ b/core/lib/Drupal/Core/Entity/EntityDisplayModeBase.php
@@ -29,7 +29,7 @@ abstract class EntityDisplayModeBase extends ConfigEntityBase implements EntityD
    *
    * @var string
    */
-  protected string $description;
+  protected ?string $description;
 
   /**
    * The entity type this form or view mode is used for.
@@ -129,8 +129,8 @@ protected function urlRouteParameters($rel) {
   /**
    * {@inheritdoc}
    */
-  public function getDescription(): string {
-    return $this->description;
+  public function getDescription(): ?string {
+    return $this->description ?? NULL;
   }
 
 }
diff --git a/core/lib/Drupal/Core/Entity/EntityDisplayModeInterface.php b/core/lib/Drupal/Core/Entity/EntityDisplayModeInterface.php
index 4c8e12639b..250bd12634 100644
--- a/core/lib/Drupal/Core/Entity/EntityDisplayModeInterface.php
+++ b/core/lib/Drupal/Core/Entity/EntityDisplayModeInterface.php
@@ -33,6 +33,6 @@ public function setTargetType($target_entity_type);
    * @return string
    *   Returns display mode description.
    */
-  public function getDescription(): string;
+  public function getDescription(): ?string;
 
 }

Please don’t assign tickets to yourself unless a maintainer. A comment should do.

@sakthi_dev Sorry but those changes are not helpful, and are the wrong direction from what I described above.

Furthermore, @Utkarsh_33 has this issue well in-hand at this point.

Reverting the above, and applying my suggestions

Went through all of the recent changes and the code. The upgrade path looks solid now. I'm a bit surprised how much complexity type hints add to the upgrade paths 😅

Some nitpicks in the docblock of the new method, and there is a merge conflict, otherwise this looks ready to go to me.

Applied the suggestions and rebased the MR.

Somehow the rebase has lost most of @tim.plunkett's changes from this commit: https://git.drupalcode.org/project/drupal/-/merge_requests/4135/diffs?co...

The changes lost with the rebase were successfully recovered by @lauriii

After a few un-RTBC at the final stretch, this looks all set and I've merged it to 11.x. Thanks all!