New option for Views page displays to use the admin theme

2719797-158.patch worked perfectly for me on 9.4.8. Thanks.

Same for me thanks!

Putting back to RTBC from #161 as there was no code change.

The last submitted patch, 158: 2719797-158.patch, failed testing. View results →

Tested the patch manually with Drupal core 10.1.x. The patch was applied cleanly and was able to use the option of the Admin theme. Attached screenshot for reference.

Moving the status to RTBC.

This has received multiple iterations of Usability review. All of the feedback has been addressed except explaining what other consequences there are from marking a page as admin page. The example provided there was Quickedit, which has been removed from core. I believe this might be outdated and therefore I'm not opening a follow-up issue for this.

1. +++ b/core/modules/views/config/schema/views.display.schema.yml
   @@ -68,6 +68,9 @@ views.display.page:
   +    use_admin_theme:
   +      type: boolean
   +      label: 'Use the administration theme when rendering the view page'
   

   Since this is no longer a required property, it should be marked as nullable.

2. +++ b/core/modules/views/src/Plugin/views/display/Page.php
   @@ -244,6 +248,27 @@ public function optionsSummary(&$categories, &$options) {
   +      $admin_theme_text = $this->t("Yes (admin path)");
   ...
   +      $admin_theme_text = $this->t("Yes");
   ...
   +      $admin_theme_text = $this->t("No");
   

   Super nit: s/"/'

3. +++ b/core/modules/views/src/Plugin/views/display/Page.php
   @@ -244,6 +248,27 @@ public function optionsSummary(&$categories, &$options) {
   +      'title' => $this->t('Admin theme'),
   ...
   +      'desc' => $this->t('Use the administration theme when rendering this display.'),
   
   @@ -445,6 +470,21 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
   +          '#description' => $this->t('Paths starting with "admin/" use the admin theme even when this option is not checked.'),
   

   We should use the term "administration theme" consistently across all of these strings instead of "admin theme"

4. +++ b/core/modules/views/src/Plugin/views/display/Page.php
   @@ -445,6 +470,21 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
   +          '#title' => $this->t('Always use admin theme'),
   

   We are missing the before admin theme.

Addressed my feedback and added some more test coverage for the UI.

New changes look good.

I ran the new test locally too to make sure it failed

Behat\Mink\Exception\ResponseTextException : The text "Administration theme: No" was not found anywhere in the text of the current page.

Excited for this new feature.

+++ b/core/modules/views/src/Plugin/views/display/Page.php
@@ -445,6 +470,21 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
+        $form['use_admin_theme'] = [
+          '#type' => 'checkbox',
+          '#title' => $this->t('Always use the administration theme'),
+          '#description' => $this->t('Paths starting with "admin/" use the administration theme even when this option is not checked.'),
+          '#default_value' => $this->getOption('use_admin_theme'),
+        ];
+        $display_path = $this->getOption('path');
+        if (!empty($display_path) && stripos($display_path, 'admin/') === 0) {
+          $form['use_admin_theme']['#default_value'] = TRUE;
+          $form['use_admin_theme']['#attributes'] = ['disabled' => 'disabled'];
+        }
+        break;

The use of "Always" here was question by @pfrenssen in #56 and sent back for usability review but the UX review in #61 did not actually address this - it made other recommendations. I think the "always" perhaps was more relevant when this was in the path setting dialog.

Furthermore the description is odd because it says Paths starting with "admin/" use the administration theme even when this option is not checked but we have code below disabling and setting the field to TRUE when the path starts with admin so it will never be not set.

I think we should change this code element to:

        $form['use_admin_theme'] = [
          '#type' => 'checkbox',
          '#title' => $this->t('Use the administration theme'),
          '#default_value' => $this->getOption('use_admin_theme'),
        ];
        if (str_starts_with($this->getOption('path') ?? '', 'admin/')) {
          $form['use_admin_theme']['#description'] = $this->t('Paths starting with "admin/" always use the administration theme.');
          $form['use_admin_theme']['#default_value'] = TRUE;
          $form['use_admin_theme']['#attributes'] = ['disabled' => 'disabled'];
        }

Updated as described in #173

added updated patch fixed CCF #174

@catch think your points have been addressed.

+++ b/core/modules/views/src/Plugin/views/display/Page.php
@@ -244,6 +251,27 @@ public function optionsSummary(&$categories, &$options) {
+    if (!empty($display_path) && stripos($display_path, 'admin/') === 0) {

@@ -445,6 +482,24 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
+        if (str_starts_with($this->getOption('path') ?? '', 'admin/')) {

I think the path should be case-insensitive. eg, /admin, /Admin or /ADMIN etc. should use the administration theme automatically. So str_starts_with should be replaced with stripos above. And we need to add a test for this.

NR for agreement.

So excited this is so close to being done! Thanks to everyone who has moved it forward.

Alas #176 introduced a ton of irrelevant whitespace changes, and undesirable capitalization changes. Un-saving "credit" for the anti-contribution.

And #174 did some things to #171 that were not requested in #173 (like removing the #title of the form itself).

I think we need to go back to the patch in #171, and carefully re-address #173 only.

I'm a bit torn on #179, but yeah, we should probably fix that, too. 😅

Here's exactly #171 with only the suggested changes in #173. I'm leaving #179 out of it for now.

+++ b/core/modules/views/src/Plugin/views/display/Page.php
@@ -244,6 +248,27 @@ public function optionsSummary(&$categories, &$options) {
+    if (!empty($display_path) && stripos($display_path, 'admin/') === 0) {

I think str_starts_with is better because it's consistent with \Drupal\system\EventSubscriber\AdminRouteSubscriber::alterRoutes. Can we change this too to use str_starts_with?

Okay, thanks for clarifying. I was about to post test-only and full to address #179 but d.o flagged it as a x-post that you had already changed the status. 😅 I'll remove the remaining stripos().

Fixes #182 (and the ?? improvement suggested for a similar code block in #173) and now core/scripts/dev/commit-code-check.sh --cached is passing. 😅

(Side note: Did something with d.o CI change in the last 3 days, or does the bot not run the same checks for core committer patches as it does for the rest of us? 😂 @lauriii's patch in #171 has all the same CS bugs as #181, but the testbot flagged mine and wouldn't even test it, but ran the whole test suite for 171... weird)

1. +++ b/core/modules/views/src/Plugin/views/display/Page.php
   @@ -445,6 +469,20 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
   +          $form['use_admin_theme']['#description'] = $this->t('Paths starting with "admin/" always use the administration theme.');
   

   Just noticed a possible minor nit:

   The code here is totally hard-coded. Do we want to let translators potentially change this? In some other UI strings, I've seen placeholders used to ensure that the path output is always what core wants. Do we care?

2. +++ b/core/modules/views/tests/src/Functional/Plugin/DisplayPageWebTest.php
   @@ -148,6 +148,37 @@ public function testPagePaths() {
   +  public function testAdminTheme() {
   

   Oh, and more uber-nit, doesn't this want : void?

3. +++ b/core/modules/views_ui/tests/src/Functional/DisplayPathTest.php
   @@ -286,4 +288,56 @@ public function testDefaultMenuTabRegression() {
   +   * Tests the "Always use the administration theme" configuration.
   

   That's not what the UI label is, anymore... 😅

I'll clean up 2 and 3 while waiting for feedback on 1.

Fixes #186 points 2 and 3 for now. Maybe point 1 isn't real.

Argh, sorry for the noise! I spotted one more "Always" we don't want.

@dww IMO providing a placeholder for the "admin/" part would be great! 👍

Cool, then this fixes #186.1. Anything else? Thanks! 🙏

Looks great! Thank you @dww!

Great, thanks! Fixed the summary and screenshots to reflect the current patch.

However, I noticed the draft CR is stale, with stale screenshots, "Always", etc. But it's almost 1am here now, so I'm just going to add the tag and hope someone else can handle that. ;)

Thanks again, excited to see this almost in core! 🎉🙏
-Derek

Hiding obsolete files and updating remaining tasks.

Committed 4dbf3f4 and pushed to 10.1.x. Thanks!

Hopefully got the issue credit correct. I credited everyone who provided screenshots of the feature because it was hard to decide whose screenshots where useful and whose where not.

Sing Halleluyah!!!!

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

I am attaching a working patch for this issue.