Problem/Motivation

When extending configuration entities, such as in #1952394: Add configuration translation user interface module in core, we need a reliable way to add additional local tabs to pages such as config entities. If some have a default edit tab and others don't, then we need to augment that from the outside. This would be way easier (also for contrib modules to extend the UI) if all config entities would have an Edit/Configure (as appropriate) tab by default.

Reviewing the complete list of config entities:

  • block: already has such a tab but was not visible on the page, only inline; so if you went to the delete tab, there was no tab to go back to configure - woops :D
  • custom_block_type: already has it
  • contact_category: already has it
  • node_type: already has it
  • date_format: in patch
  • filter_format: in patch
  • image_style: already has it
  • language_entity: in patch, although /edit.../edit looks odd, language editing should be converted a controller and the paths fixed THEN
  • menu: already has it
  • picture_mapping: in patch
  • shortcut_set: already has it
  • taxonomy_vocabulary: already has it
  • user_role: already has it
  • view: already has it

Proposed resolution

Add the Edit/Configure (as appropriate) tabs.

Remaining tasks

Review.

User interface changes

The tab will show up *IF* there is at least one other tab. If there is no other tab, the tab will not show up, so it will look exactly like before. So in case of the entities covered in the patch there is *NO* core UI change until #1952394: Add configuration translation user interface module in core lands and you have that enabled. That adds one more tab for each config entity, so the default tab will also show.

API changes

None.

Related Issues

CommentFileSizeAuthor
#21 after-nodelete.png144.79 KBYesCT
#20 default-edit-tabs-20.patch3.01 KBYesCT
#9 withoutpatch-deleteblock.png144.9 KBYesCT
#9 withpatch-deleteblock.png126.33 KBYesCT
#9 configure-block.png461.91 KBYesCT
#9 custom-block.png424.67 KBYesCT
#8 date-noedit.png156.64 KBYesCT
default-edit-tabs.patch3.16 KBGábor Hojtsy
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Status: Needs review » Reviewed & tested by the community

Awesome idea!

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Issue tags: -D8MI, -sprint, -language-config

default-edit-tabs.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, default-edit-tabs.patch, failed testing.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Needs work » Needs review
Issue tags: +D8MI, +sprint, +language-config

default-edit-tabs.patch queued for re-testing.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +blocker

Was a test fluke, back to RTBC then. Marking as blocker for massive DX improvement in #1952394: Add configuration translation user interface module in core.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented

+1 to translatable and configurable forms #2087279: Add a config() method to FormBase

YesCT’s picture

YesCT CreditAttribution: YesCT commented

default-edit-tabs.patch queued for re-testing.

YesCT’s picture

YesCT CreditAttribution: YesCT commented
FileSize
date-noedit.png156.64 KB

date format is indeed the same before and after (no edit tab)

date-noedit.png

I checked the others also.

the coding style and such looks fine.

I'm not sure I understand this change... I could not get to a block ... page ... which had a delete tab.

+++ b/core/modules/block/block.module
@@ -115,7 +115,7 @@ function block_menu() {
-    'context' => MENU_CONTEXT_INLINE,
+    'context' => MENU_CONTEXT_PAGE | MENU_CONTEXT_INLINE,

.

YesCT’s picture

YesCT CreditAttribution: YesCT commented
FileSize
custom-block.png424.67 KB
configure-block.png461.91 KB
withpatch-deleteblock.png126.33 KB
withoutpatch-deleteblock.png144.9 KB

ohh

without patch: at admin/structure/block/manage/bartik.help/delete

withoutpatch-deleteblock.png

with the patch:

withpatch-deleteblock.png

similar for configure block

configure-block.png

works for custom blocks too

custom-block.png

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented

Yes, the bikeshed in #1834002: Configuration delete operations are all over the place

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented

Yes, the bikeshed in #1834002: Configuration delete operations are all over the place

Bojhan’s picture

Bojhan CreditAttribution: Bojhan commented
Status: Reviewed & tested by the community » Active

What the hell? We fighted really hard to not make these things tabs. If this gets in, we rather just not bother with user research driven improvements + documentation after that if it's ignored so clearly. You can't just call bikeshed, because I don't agree with you. This will be a significant decrease in usability, and I would almost mark this won't fix.

klonos’s picture

klonos
he/him
Primary language English
Location 90% Melbourne, Australia - 10% Larissa, Greece
CreditAttribution: klonos commented

I don't know about Edit/Configure, but I was under the impression that Delete was going to be moved to a modal. If I got it right then, there'll be no delete tab. But with #1952394: Add configuration translation user interface module in core and also contrib adding tabs, this whole thing seems like a good idea to me too like others. So, I'm interested in what @Bojhan is talking about in #12 above. Care to explain further? Where did this "fight" take place? What were the arguments against doing it and are they still valid?

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Active » Reviewed & tested by the community

@Bojhan: look, you got confused by the screenshots of what is going on in this patch. The goal of this patch is to add a default Edit/Configure tab. All the other tabs show up are not touched/changed by this patch. The fact that the Delete tab shows up when you put a configure tab on a block is due to how **the delete tab** is defined. When there is only one tab defined on a page, it will not show up (like YesCT verified for date formats by default). Once there are more tabs, they all show up. Blocks **currently** **in core** only have a **delete** tab defined. They don't have an **edit/configure** tab. It is not the goal of this issue to remove or in any way fiddle with **delete** tabs. If a module (like config translation) puts on a tab on blocks, the delete tab will equally show up, but then you'll have that delete tab, and the additional tab added by config translation but no tab to actually go back to the editing form. *That* is the problem being resolved in this patch. **This patch has nothing to do with how or if delete tabs are defined**.

(Once in another issue that delete tab is removed again, there will be no tab showing on blocks by default because again, there will only be one tab, the configure tab. I'm happy to work with you in #1834002: Configuration delete operations are all over the place to make that delete tab not show up. Blocks are not the only place where you get delete tabs btw.)

All-in-all moving back to RTBC.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs usability review

Hm, sorry, but it sounds like in any event this patch still needs usability review.

It might be that we should postpone it on something that deals with the way deletes are defined so we don't end up with a UX regression.

Bojhan’s picture

Bojhan CreditAttribution: Bojhan commented
Issue tags: -Needs usability review

I am all for making edit/configure tabs show up - but not when it creates such a big UX regression all over the place. Because so far, really no one showed interest in solving that. I fear that if this gets in, and no one works on the delete tabs - that this patch will introduce a regression that is not solved before release. So either we increase criticality or some support for the new issue.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented

I don't see this as a UX regression, because custom block types, contact categories and image styles already have always visible delete tabs in core. And this is only from config entities, not even considering content entities, where it is also similarly inconsistent. The only config entity where this patch makes a delete tab show up is block placements. None of the others will have a delete tab appear with this patch. The three I mentioned *already have them* visible. So I don't see how this is any reasonable size of a change in the larger scheme of things. After this patch (as a side effect!), out of the 14 listed config entities, 4 would have a delete tab showing up, whereas *before* the patch 3 had.

I've posted a patch to remove all delete tabs and unify them by only defining them in routes so people don't get ideas about putting them into tabs: #1834002-46: Configuration delete operations are all over the place.

I think it may be dangerous to block a developer experience improvement of a new feature not even in core yet with a UX issue that is still being largely debated (which is what @andypost referred to as bikeshed IMHO). If you read that issue, you'll see people just don't agree. I can support with any number of patches but if you all don't come over and get it in, then this will not be useful and we'll need to be back to specify in config translation if an edit tab needs to be added, or add more magic to generate them. (And that would equally surface the block placement delete tab *anyway*).

I think its unfortunate that the screenshot example above was from block placements because that is the only config entity in core, which has a "sleeping delete tab" which would appear if more tabs appear. There are three other entities which have a delete tab alive and well and none of the other entities have it as a (anytime visible) tab.

YesCT’s picture

YesCT CreditAttribution: YesCT commented

Yeah, this is only showing a tab that already existed... any other contrib that added a tab to that would also cause the delete to show. This did not add the delete tab. *and* it is only in *one* place, for blocks. (maybe because I showed it from both the configure and delete tabs and a block that was a custom block it looked like more than just one place.)

YesCT’s picture

YesCT CreditAttribution: YesCT commented
Issue tags: +Needs screenshots

#1834002: Configuration delete operations are all over the place had a commit to take off the config delete tab.

new screenshots should make this one ok. adding tag. (if someone starts this, post a comment so we dont duplicate work.)
https://drupal.org/contributor-tasks/add-screenshots

YesCT’s picture

YesCT CreditAttribution: YesCT commented
FileSize
default-edit-tabs-20.patch3.01 KB

reroll, so no interdiff. (the https://drupal.org/patch/reroll instructions didn't work moving back for 1834002, so I just redid by hand)

diff stats the same:
Diff statistics
5 files changed, 21 insertions, 1 deletions.

YesCT’s picture

YesCT CreditAttribution: YesCT commented
Issue tags: -Needs screenshots
FileSize
after-nodelete.png144.79 KB

before and after the patch in #20 there is no delete tab.

admin/structure/block/manage/bartik.help

after-nodelete.png

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Needs review » Reviewed & tested by the community

In that case back to RTBC as per #15 since we averted the regression :)

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Fixed

Committed e560115 and pushed to 8.x. Thanks!

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented

I'm not 100% sure this should have a change notice, it is a best practice really. #2089757: Auto generate routing entries for entities based on an 'admin_path' and 'admin_permission' annotation wants to autogenerate these routes (and hopefully tabs as well), so it will become a best practice in an easy way hopefully, making DX much simpler.

YesCT’s picture

YesCT CreditAttribution: YesCT commented

Same thing on other pages. #2095117: Menu system should provide a default tab if none exists

YesCT’s picture

YesCT CreditAttribution: YesCT commented
Issue summary: View changes

added related issues

YesCT’s picture

YesCT CreditAttribution: YesCT commented
Issue summary: View changes

created the core issue

YesCT’s picture

YesCT CreditAttribution: YesCT commented

we can use this on config translation issue: #2095269: Remove add_edit_tab, core should have default tabs

YesCT’s picture

YesCT CreditAttribution: YesCT commented
Issue tags: -sprint

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

(not verified) CreditAttribution: commented
Issue summary: View changes

added config trans issue