[UX] Add back admin bar links to manage display sub-pages

[jenlampton]

While working on https://github.com/backdrop/backdrop-issues/issues/315 we removed the menu router items for the sub-tabs on manage displays. This also effectively removed them from the admin bar. We should add them back or there will be a little UX regression.

[quicksketch]

There are lots of errors being reported by testbot currently.

[jenlampton]

Bah, will take a look.

[jenlampton]

I resolved all the PHP notices, and added the bundle and view-mode names into the admin bar links so each would be unique.

---

Adding milestone candidate tag for 1.11.4

[klonos]

I have not reviewed the code yet, but here's my comments from the PR sandbox:

• Display names in the menu seem to be using the machine name. I think that they should use the human-readable name instead.

• The "Enable display" menu item is available for already enabled displays.

• Menu items use "configure", while the operation is called "manage" on the listing page ("configure" seems to be used for custom/added displays).

• "Delete" is not available as an operation on the listing page for default/existing displays, yet shown as a menu item for them.

• I have enabled a previously disabled display, but it did not show up on the admin menu. Clearing caches did not seem to help.

• I have created a new display, but it did not show up on the admin menu. Clearing caches did not seem to help.

[jenlampton]

Display names in the menu seem to be using the machine name. I think that they should use the human-readable name instead.

Updated.

The "Enable display" menu item ...

removed the enable option entirely, I don't think that's something people will be doing from the admin bar.

"Delete" ...

removed the delete option entirely, I don't think that's something people will be doing from the admin bar.

Menu items use "configure", while the operation is called "manage" on the listing page ("configure" seems to be used for custom/added displays).

Since the top-level link and the manage/configure link lead to the same place, I removed the configure link as well as enable and delete. My guess is that 99% of the time people will want to get to the manage/configure page and those other options can be reached from the "list" page.

I have enabled a previously disabled display ... created a new display, but it did not show up on the admin menu. Clearing caches did not seem to help.

I've added both an admin_bar cache flush, in addition to the menu_cache_clear() when displays are created, deleted, enabled, or disabled.

This is ready for another manual test :)

[stpaultim]

@jenlampton - Sandbox is not working. Please, refresh.

[klonos]

@stpaultim I've started off with @jenlampton's existing PR, fixed the conflicts and rebased with the current 1.x, then fixed more things. Here's a new PR against 1.x core: https://github.com/backdrop/backdrop/pull/4377 (so that we can get a fresh tugboat sandbox to play with + run tests against our current workflows instead of ZenCI).

@jenlampton Ive tried to create a PR against yours, but it was crazy! 😅 ...so many things have changed in the last 2.5+ years, that the commit history alone was unmanageable.

I tried to push smaller commits often, to make it easier to review. Here's a summary of what I've changed:

• reverted the changes in core/modules/admin_bar/admin_bar.inc, since we'll be dealing with that in #4586
• reverted the changes in core/modules/file/file.module as I found them to be irrelevant (they seem to have been reverting intentional more current changes in core)
• added some consistency in the way rows are being created for the table in theme_field_ui_view_modes() (which by the way was broken when I started to work on this)
  • made it so that row descriptions are being pulled from the 'status' key provided by field_ui_get_view_mode_data() (which by the way I've left mostly untouched - only wrapped the strings around t() cause you missed that in your PR), instead of adding another set of strings that are almost identical.
  • all descriptions are now <div class="description"> (the one for the non-customized displays was previously wrapped in <em>)
• streamlined the field_ui_manage_display_link_title() callback to be using the existing field_extract_bundle() and entity_get_info() functions + field_ui_get_view_mode_data() (which you've introduced with your PR, and which I've left unchanged). This also fixed the issue with the default display always being lowercase (Page: default display vs Page: Teaser display, Page: Full content display etc.), because we are now using the label of the default display instead of its machine name.

I think that's it - I hope I didn't miss anything important. The sandbox now works, so we can play with it.

Things that I would like us to consider changing:

1. The labels for the menu items are currently being prefixed with the name of the entity type, which I think we should remove. We currently have Structure -> Content types -> Card -> Manage fields -> Body (not ... Manage fields -> Card: Body), so I don't think that we should be doing Structure -> Content types -> Card -> Manage displays -> Card: Default display. It should be just ... -> Manage displays -> Default display (without the prefix). In fact, I think that we should also ditch the "display" suffix as well, since we are not adding that to fields (consistency please).
2. It's nice that we are adding the admin bar links, but there were also secondary horizontal tabs, that allowed people to navigate between displays. Compare Backdrop vs. D7: Sure, we could leave those for a follow-up, but it really feels that they are part of this.
3. Similar to the previous point, we could really use a "Back to the display modes list" or just "Cancel" button (only when you have navigated to a specific display edit page from the listing page that is).
4. The "Manage displays" menu items are siblings of the respective "Manage fields" menu items when it comes to content types, vocabularies, but for user accounts it is a child item. That's really inconsistent/annoying.
5. There is already a "Manage file display" menu item for file types, which lists the displays (but without the prefix). These must have been added in the meantime from the time this PR was created. Should we:
   • remove file-type-specific items in favor of the ones we are adding here (for consistency)?
   • or instead exclude file types from our PR here? If we decide to do this, then do we add the "Add display mode" item to the existing "Manage file display" menu items, or not?

[jenlampton]

Related: https://github.com/backdrop/backdrop-issues/issues/4031 move contents of "Manage file display" page

[stpaultim]

Is this something we can get into 1.25.0? @jenlampton or @klonos ?

The horizontal tabs would be nice. Is that a different issue/PR or does that really belong here?

Here are screenshots on one example:

WAS:

NEW:

[klonos]

@stpaultim this is currently labeled as a task, so it can go in anytime ...unless we deem it to be too big a change, and we only merge into 1.x (for the next minor release).

I currently need:

• Feedback and review of the bullet points I have implemented (good thing? bad thing? leave out of this PR and into a separate issue?).
• Feedback/suggestions on the numbered points I have listed as to-do (do people agree or not when it comes to these proposed changes? part of this issue here, or separate discussion?).

[bugfolder]

Feedback and review of the bullet points I have implemented Feedback/suggestions on the numbered points I have listed as to-do

I'll defer feedback on the bullet points (don't think I can add much to those implementation issues), but I will comment on the numbered queries:

1. The labels for the menu items are currently being prefixed with the name of the entity type, which I think we should remove.

I agree; ditch both "Card:" and "display" for consistency with the "Manage fields". (I very much like this ability to go straight to the relevant display.)

2. It's nice that we are adding the admin bar links, but there were also secondary horizontal tabs, that allowed people to navigate between displays.

I miss those from D7. They would be great to put back here. Especially because the tabs for the "Manage Displays" page and the "Manage (a specific display)" page look exactly the same right now, and they would benefit from some visual distinction in the top-of-page tabs.

3. Similar to the previous point, we could really use a "Back to the display modes list" or just "Cancel" button.

This would be nice, although my inclination to get back to the listing is to click on the "Manage displays" tab, so I don't think it's strictly necessary. But it doesn't hurt to have.

4. The "Manage displays" menu items are siblings of the respective "Manage fields" menu items when it comes to content types, vocabularies, but for user accounts it is a child item.

Agreed. (It also seems a little odd that "Manage fields" for user accounts is in Configuration but Manage fields for content types is in Structure. They both seem structure-ey.)

5. There is already a "Manage file display" menu item for file types, which lists the displays (but without the prefix). ... Should we:

No strong opinion here.

[klonos]

Thanks @bugfolder 🙏🏼 ...I'll wait for some more feedback from others, and I'll then start working on this again.

IIRC, I have actually tried to restore the secondary horizontal tabs a few weeks ago, but it didn't work and it didn't seem straight forward either. I vaguely recall @jenlampton mentioning something about horizontal tabs being broken in a higher level - not only here, but I can't remember details about it (or perhaps not remembering things correctly at all). @jenlampton does this ring any bell? Do you have any details, or any issue we have already for that?

[olafgrabienski]

I'll wait for some more feedback from others, and I'll then start working on this again.

Thanks for your work on the admin bar links and the secondary tabs! I've been missing both and go along with @bugfolder's feedback.

[argiepiano]

Thanks for this! I also miss the ability to go directly to a specific display mode edit page from the admin bar. I agree with @bugfolder's points. I would add, however, that not having secondary tabs for all enabled display modes shown in the edit page is an issue for me. Like @bugfolder, I often have to customize more than one display mode at a time, and not being able to switch easily through those secondary tabs is a problem (both currently, and with this PR). So, I would vote for trying to bring those tabs back.

One more point. Backdrop removed a fieldset shown below the field table in the Display mode edit page (see below).

That was moved to the intermediary page found at admin/structure/types/manage/page/display where all display modes are shown (including those disabled ones). This adds an extra step for Backdrop site builders (you have to visit that page, then click "Manage display" for the specific display mode). This PR makes it easier to skip that intermediary step - you can go directly to the edit page for a specific display mode. The problem with this is that it's now easier to miss the fact that there are a bunch of disabled display modes. That said, I'm not sure what the solution may be.

[klonos]

The problem with this is that it's now easier to miss the fact that there are a bunch of disabled display modes. That said, I'm not sure what the solution may be.

I have noticed this less-than-ideal UX workflow as well @argiepiano, and I too don't know how to address it the best way. It kinda feels like it deserves an issue of its own, but I'll have a think re whether we can improve things as part of this here.

[stpaultim]

@klonos

I've looked at the sandbox and read your suggestions for additional improvements. I like what you have done so far and don't have anything to add to the points made by @argiepiano and @bugfolder.

I do think that these points are expanding the scope of the issue quite a bit, which is fine with me if you are motivated to get them done in this PR. BUT, I think you have already accomplished what the original post asked for, so should we get that in and create follow-up issues?

[klonos]

I think you have already accomplished what the original post asked for, so should we get that in and create follow-up issues?

Certainly yes @stpaultim ...I am also a big fan of "baby steps" and iterative improvements. We can do follow-up issues for sure 👍🏼 ...BUT, I would at the very least like to fix the inconsistency of the menu link labels (remove the prefixes and suffixes). Then we should be good to go 😉

[argiepiano]

Any chance to get those sub-tabs to jump to other display modes working, though? That seems to me closely related to this functionality.

[klonos]

@argiepiano I have tried but could not figure it out. Apparently there's a deeper issue with secondary tabs. As I said in an earlier comment:

... I vaguely recall @jenlampton mentioning something about horizontal tabs being broken in a higher level - not only here, but I can't remember details about it ...

In the meantime, I have removed the prefix/suffix from the menu link titles + fixed a couple of typos reported by the cspell check. I will temporarily switch to work on #6049, as the failures in the PR are only because of that.

If you like, please feel free to pull my PR on your local and give it a go to see if you can figure out how to add back the horizontal secondary tabs. If you do, then we can combine everything into a single PR - if not, then a follow-up issue I guess.

[klonos]

Update: I fixed the PR failures, and it was actually a simple fix. However, there's room for improvement, but when I tried that, I started getting all sorts of other failures. So I've left it for #6049. Please read my last comment over in that issue for details.

So we need code reviews, testing, and feedback on the current PR. Setting the "help wanted" label for people to see if they can figure out the problem with the secondary tabs.

[argiepiano]

If you like, please feel free to pull my PR on your local and give it a go to see if you can figure out how to add back the horizontal secondary tabs.

I'm working on this. Will report back tomorrow,.

[argiepiano]

Hi @klonos. First of all thank you and @jenlampton for all the work on this issue.

Rather than tinkering with what you had, I decided to submit an alternative PR, since my approach is very different from your PR. The changes in my PR are smaller, and they take advantage of Backdrop's own capabilities to create secondary tabs AND menu items for the admin bar, rather than manually tinkering with the admin bar.

This is how this PR is different:

It creates menu router items for each display mode. This is the only way for the local task functionality to really work (see menu_local_tasks(), which is responsible for creating secondary tabs). The current approach in core is to create a single menu router item with a placeholder for the view modes. This is in some ways more efficient, BUT has the drawback of not working for the secondary tabs functionality and admin bar link functionality, as you discovered.

A couple of additional points: this approach (my PR) is the way Drupal 7 dealt with these display mode secondary tabs. So the approach is “tried and true”. It does add a few more router items to the database, but these are not that many (basically the router item paths for nodes, regardless of bundle, looks like this: admin/structures/type/manage/%/full, admin/structures/type/manage/%/rss, etc.)

So this PR, with very few changes, takes care of both requests in the discussion: it creates working the secondary tabs, AND it includes those view modes in the admin bar. It simply lets Backdrop do its job by correctly defining menu local tasks the way they were intended.

[klonos]

This is so awesome @argiepiano! ❤️ 🙏🏼 ...can't wait to review and test it (and by the looks of it close my PR over yours).

[klonos]

...The changes in my PR are smaller, and they take advantage of Backdrop's own capabilities to create secondary tabs AND menu items for the admin bar, rather than manually tinkering with the admin bar.

SO-SOOO much simpler/smaller!!! Kudos again @argiepiano 👍🏼 ...this is one of those "How didn't I think of that!" moments 😅

[klonos]

...OK, quick testing/review is very positive 👍🏼 ...here's what feels odd:

1. The horizontal tabs are shown in the main "Manage display" tab, which would otherwise be OK, but we already have individual "Manage display" action buttons. So should we remove them from the main display listing? 🤔
2. Related to the above: if you are managing a thing that has only a single display enabled/customized, there are no secondary tabs at all - they only appear if you enable more than one display. You can see that by default in admin/structure/types/manage/post/comment/display for instance.
3. When managing a display, there is no obvious way to "go back". Sure, seasoned Drupal/Backdrop users will know to click the "Manage displays" primary tab, but it feels that we need some "cancel" link that takes people back to the list as well. ...although, having said that, a "cancel" link would make sense only if you have found yourself in the manage display form from the display listing page. If you were in a totally irrelevant page, and navigated to the display edit form via the admin menu, then "cancel" won't make much sense ...where would it take you? 🤔 ...perhaps a follow-up?
4. Specifically for the display listing of file types: it feels inconsistent how the "Manage displays" tab is a listing, whereas the "Manage file display" takes you to the first available display instead of showing you a list. Again, not sure if/how to solve this tiny UX inconsistency 🤔

[argiepiano]

@klonos, thanks for testing and for the observations!

I've pushed a couple of changes that add an "Overview" secondary tab. This seems to me helpful to address points 2 and 3 of your observations. Now, even when you have only one enabled view mode, you will see an "Overview" tab and a "Default" tab (see below). Also it's helpful as a way to go "back" or "up" to the "Manage display" screen when you are editing a view mode.

As for point 1, I think the addition of "Overview" makes the fact that those tabs appear in the overview page less confusing. It's easier now to see that they act as "quick shortcuts" to the actual view mode edit pages.

As for point 4: that's not addressed in the issue. Those consistency issues already existed in core. Perhaps a separate issue for those? I also see some inconsistency in the admin bar links for user's Manage fields and Manage display (displays is shown inside fields in the admin bar menus). So, that also needs to be addressed... in a separate issue?

[argiepiano]

Is this something we may want to include in the next minor release, @klonos?

[klonos]

@argiepiano this is actually a UX task - not a feature. So it can get in with any bug fix release 😉 ...but yes, I would love for it to be done with the next minor release.

[klonos]

@jenlampton can you please close your PR? (to avoid confusion with which one people should be reviewing/testing)

Thanks in advance.

[jenlampton]

PR closed :D

I thought we had a dedicated issue for missing secondary tabs, but I might have been thinking of https://github.com/backdrop/backdrop-issues/issues/2681

[izmeez]

The PR https://github.com/backdrop/backdrop/pull/4407 has expired, does it need to be rebased for testing?

[izmeez]

Thanks. The PR is working and it's great to be able to see the full depth of the admin bar menus, again or a new. You don't know what you missed til you get it back! So the PR definitely works for me.

Then taking a look at the code changes, if I add to end of the PR url either .diff or .patch and am surprised to see different results.

With the .diff extension of the url there is a much shorter rendition only with a few changes to the core field_ui module.

By comparison, with the .patch extension of the url there are more changes to the core field_ui module and also changes to core common.inc file.

I'm not sure which of the two should be reviewed? Is the .diff all that's needed or just the last commit and the .patch a consolidation of all the commits in the PR?

Just trying to clarify my understanding. Thanks.

[bugfolder]

Is the .diff all that's needed or just the last commit and the .patch a consolidation of all the commits in the PR?

Yes, exactly.

[izmeez]

@bugfolder The question has an OR in it so which is correct? Thanks.

[argiepiano]

It's strange that the patch file shows stuff unrelated to this PR. I think it has to do with my fork.

It's always best to review the code by clicking the "Files changed" link. But if you really want to check the diff, you can do that. The patch file is not correct.

[argiepiano]

I think the patch file is showing a commit I made by mistake in the main branch of my fork, that I later reverted. So, the patch file is showing both, the wrong commit and the reverted. I would need to delete my fork and fork again to remove those two commits, but that would remove PRs I made that haven't been yet merged.

[bugfolder]

The .diff is all that's needed and the .patch is a consolidation of all the commits in the PR.

[bugfolder]

If changes were made, then reverted (and/or further altered), the .patch will show and include them. That can make it hard to see changes clearly from the .patch.

[izmeez]

If the .diff is all that's needed, that is awesome. The code changes required are simple and effective. I'll have to take one more look before saying I've reviewed the code, but it's looking good.

[izmeez]

It's always best to review the code by clicking the "Files changed" link. But if you really want to check the diff, you can do that. The patch file is not correct.

Well that can be a way to confirm but it's not so useful for grabbing a patch for local testing.

[argiepiano]

Well that can be a way to confirm but it's not so useful for grabbing a patch for local testing.

Both the patch and diff files will result in the same code after applying them. The patch file contains changes and reverted changes that have no effect on the final changes.

[izmeez]

Yeah, but it what my brain has to go through and totally unnecessary. Just to experience your own mental gyrations. I'm okay without that. Elegance and simple is nice. Isn't that want @klonos was thinking when he said he wished he had come up with it.

[argiepiano]

Those false changes and reverts are not related to this PR - they originate from a different issue from two years ago. My fork kept those, and they appear in every new PR unfortunately.

[izmeez]

To settle the diversion I created, I took a look at a simple example of a PR I made elsewhere. From what I can see there is a difference between adding .diff or .patch to the end of the PR url. The end result of applying either as a patch will be the same. The former, the .diff version, appears to be a unified patch where all the commits are consolidated into a single one showing the total net effect. Whereas the latter, the .patch version, includes each commit as a separate patch with each commit message, showing the steps taken along the way.

Apart from this standard behaviour which I previously had not examined in such detail. The wrinkle here has been the left over spaghetti from @argiepiano forking. If you only ever use the .diff extension approach you may never even see it and have the benefit of seeing the unified end result.

[izmeez]

I have looked at the code changes and I don't think I have enough depth of understanding to comment on the coding itself. I will have to leave that for someone else. All I can say is it works well and it would be a good fix to the regression. Thank you.

[izmeez]

Meanwhile, I have applied the diff patch (as opposed to the patch patch) to my dev platform for further enjoyment. Had to flush caches, something to be expected. Thank you.

[klonos]

@argiepiano code LGTM, thanks 🙏🏼 ...I only left a comment with a suggestion.

@izmeez there were many changes in this PR that were mere formatting changes in order to make the PHPCS check happy (it complains if array declarations go beyond 80 characters, so those occurrences were wrapped in multiple lines - no actual code change).

If you don't mind using the GitHub web UI to view diffs, this is what I believe might help you:

1. when you visit a PR page, you are by default taken to the "Conversation" tab - switch to the "Files changed" tab at the top of the page instead
2. click the ± bellow the tabs row, in order to switch to a unified diff view

That will show the diff in this format, which is what you have been used to from what I understood by your previous comments:

[izmeez]

@klonos Thanks for the tip about using the Files tab as an alternative to view the diff. It also has some extra stuff showing, so it's ok for a look. But, it's not as useful to me for looking at just the diff in plain text so I can download as a patch. I can read it just fine in a plain text file. Thanks.

[izmeez]

I can see how the Files tab view is advantageous for seeing ongoing code suggestions. Although I'm not sure I fully understand how you added your code to make that happen.

[argiepiano]

@klonos, please see my comment to your last suggestion in the PR. Thanks for reviewing!