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

[jenlampton]

While working on #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.

[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!

[klonos]

Sorry for taking this long to reply @argiepiano ...RTBC 👍🏼

[izmeez]

It looks to me like this has been fixed in BD 1.27.0-preview. Is this issue ready to close?

[argiepiano]

Huh? This was not included in 1.27.0-preview.

[izmeez]

So, let me take my foot out of my mouth.
Clearly, the PR has not been merged.
I'm not sure if it also includes a test to confirm the admin bar menu displays to it's full depth.
My eyeball review of a fresh site with bd-1.27.0-preview goes down to what looks like a full depth, but maybe there is no default admin menu item deep enough?

It would be really good to see this in the 1.27.0 release.

[quicksketch]

Merged backdrop/backdrop#4407 into 1.x and 1.26.x. Thanks @argiepiano and @jenlampton for creating the PRs. And thanks to @klonos, @izmeez, @stpaultim, @bugfolder, and @olafgrabienski for the repeated reviews and feedback! It's great to close out this old issue.