Problem/Motivation

Some administrative lists and tables can be viewed in a compact mode where descriptions are hidden to reduce clutter. One example is the permissions page.

Unlike other front-end settings, this flag is stored as a cookie. A call to the system.admin_compact_page is made in order to set/unset it. This is the last usage of user_cookie_save() and is also inconsistent with other frontend only flags which are stored in localStorage.

Steps to reproduce

Proposed resolution

Option 1:
Refactor the admin compact mode into a flag in localStorage, similar to tabledrag, form user info, toolbar, etc.

Option 2:
Lightweight service that toggles the cookie

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#46 3584347-nr-bot_5j5w7o21.txt91 bytesneeds-review-queue-bot
#44 3584347-nr-bot_y3nk9utl.txt91 bytesneeds-review-queue-bot
#31 3584347-nr-bot__n7fktz4.txt91 bytesneeds-review-queue-bot
#18 3584347-nr-bot_b6ee1ubu.txt4.62 KBneeds-review-queue-bot

Issue fork drupal-3584347

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

znerol created an issue. See original summary.

znerol’s picture

znerol commented
Related issues: +#3571172: Deprecate system_sort_themes() and system_check_directory() in system module, +#3581569: Remove user_cookie_save() and user_cookie_delete()
nicxvan’s picture

nicxvan commented

I think we should take care of this here #3571172: Deprecate system_sort_themes() and system_check_directory() in system module

andypost’s picture

andypost
he/him
Primary language Russian
commented

Not sure it replacable with local storage as any controller which respects compact display of data will be affected

nicxvan’s picture

nicxvan commented

Yeah, all of the core interactions with this do it server side, I'm not sure a localStorage replacement is appropriate.

I will work on a quick MR that just converts it to a singleton service.

nicxvan opened merge request !15582

nicxvan’s picture

nicxvan commented
Issue summary: View changes

I put option1 in the MR, if you see a way to put it in localStorage without refactoring all of the code using it significantly please let me know.

nicxvan’s picture

nicxvan commented
Title: Save admin_compact_mode per user override in localStorage instead of a cookie » Deprecate and replace system_admin_compact_mode()
nicxvan’s picture

nicxvan commented
Status: Active » Needs review
nicxvan’s picture

nicxvan commented
Status: Needs review » Needs work
nicxvan’s picture

nicxvan commented
Status: Needs work » Needs review
mstrelan’s picture

mstrelan commented

Wondering if this should be an option on the user account instead, similar to how Gin theme allows accounts to configure their preferred appearance.

nicxvan’s picture

nicxvan commented

@mstrelan I had a discussion in slack with @catch and @andypost about this.

We ended up settling on moving it to session and remove the cookie entirely.

I added a new CR for this and renamed Drupal_visitor_admin_compact_mode to admin_compact_mode.

nicxvan’s picture

nicxvan commented
Status: Needs review » Needs work

I need to figure out how to manage the functional test which is using Mink's cookie management.

nicxvan’s picture

nicxvan commented
Status: Needs work » Needs review

Added credit from slack so I don't forget.

Catch, Andypost, and Godotislate helped me get the session stuff worked out.

znerol opened merge request !15633

znerol changed the visibility of the branch 3584347-admin-compact-mode-in-frontend to hidden.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3584347-nr-bot_b6ee1ubu.txt4.62 KB

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

nicxvan’s picture

nicxvan commented
Parent issue: » #3566536: [meta] eliminate core .module files
znerol’s picture

znerol commented
Issue summary: View changes
znerol’s picture

znerol commented
Status: Needs work » Needs review
Issue tags: +Needs frontend framework manager review

Tagging for frontend FM review.

znerol changed the visibility of the branch 3584347-admin-compact-mode-in-frontend to active.

nicxvan changed the visibility of the branch 3584347-save-admincompactmode-per to hidden.

nicxvan’s picture

nicxvan commented
Issue summary: View changes
nicxvan’s picture

nicxvan commented
Issue summary: View changes

jurgenhaas made their first commit to this issue’s fork.

nicxvan’s picture

nicxvan commented
Status: Needs review » Reviewed & tested by the community

I think this is ready, all of my feedback has been addressed.

This still needs front end framework signoff, but I think we can mark it now and get reviews.

I've reached out in slack a couple of times for FFM review.

quietone made their first commit to this issue’s fork.

quietone’s picture

quietone commented

I added "()" after the function and method name in the @trigger messages, per the "how to deprecate' documentation. Since that is all I did I am leaving this at RTBC.

znerol’s picture

znerol commented

Thanks @quietone.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new 3584347-nr-bot__n7fktz4.txt91 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

znerol’s picture

znerol commented
Status: Needs work » Needs review

Resolved a merge conflict in system.post_update.php.

nicxvan’s picture

nicxvan commented
Status: Needs review » Reviewed & tested by the community

Looks good, thanks!

Same caveats as 27 though.

znerol’s picture

znerol commented

I pinged accessibility folks in slack, follow-up to improve that part: #3593539: [PP-1] Replace compact mode link with a switch to improve accessibility.

kentr’s picture

kentr commented

FTR, I think this also needs an event handler for the SPACE key. It's functioning as a button in the MR, and buttons are operable by the SPACE key. But since it's still a link in the markup, that could also cause confusion.

So, unless we also change it to a button or add role="button" here, I'm not sure that waiting to add that event handler in the accessible makeover #3593539: [PP-1] Replace compact mode link with a switch to improve accessibility will make things worse.

znerol’s picture

znerol commented

I'm not sure that waiting to add that event handler in the accessible makeover #3593539 will make things worse.

It hardly can be worse than the UX in main right now. Just recall that clicking that link currently results in a whole new page load. While not perfect, at least the reading position is preserved with this MR.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Reviewed & tested by the community » Needs work

Added some review feedback, MR also needs rebasing.

znerol’s picture

znerol commented
Status: Needs work » Needs review
nicxvan’s picture

nicxvan commented

All of the feedback has been addressed.

The test changes look good, but I'm not super familiar with this type of testing.

nicxvan’s picture

nicxvan commented
Status: Needs review » Reviewed & tested by the community
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Pushed a small change that swaps SimpleXML for DOM so we can use Html::load() and use more correct XPath, leaving at RTBC while tests run.

nicxvan’s picture

nicxvan commented

I reviewed the most recent updates to the test, they look good, but same caveat as 39.

jurgenhaas’s picture

jurgenhaas
he/him
Primary language German
Location Gottmadingen
commented
Related issues: +#3606655: Make dark mode a first class citizen of Drupal core
needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new 3584347-nr-bot_y3nk9utl.txt91 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

znerol’s picture

znerol commented
Status: Needs work » Needs review

Rebase after #3586221: Remove the Stable 9 theme.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3584347-nr-bot_5j5w7o21.txt91 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

znerol’s picture

znerol commented
Status: Needs work » Needs review

Rebased after #3572350: Disable the default /node listing view, replace Olivero hardcoded welcome page from /node with welcome message

nicxvan’s picture

nicxvan commented
Status: Needs review » Reviewed & tested by the community

Rebase looks good, I bumped the deprecation from 11.4 to 11.5.

I'll keep an eye on tests.

znerol’s picture

znerol commented
Status: Reviewed & tested by the community » Needs review

Back to NR for the CSS change, also the functional test had to be adapted slightly.

nicxvan’s picture

nicxvan commented
Status: Needs review » Reviewed & tested by the community

Changes look good!

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

What happened to #41, did it get lost in a rebase? It makes the test simpler if we use DOM instead of SimpleXML:

https://git.drupalcode.org/project/drupal/-/merge_requests/15633/diffs?c...

znerol’s picture

znerol commented

Uh, I have no idea how this happened. I've cherry-picked the commit. For reference:

git fetch drupal-3584347 f969980d20f428a88aab2d9d6973e945a425cb94
git cherry-pick f969980d20f428a88aab2d9d6973e945a425cb94
nicxvan’s picture

nicxvan commented

Weird, yeah that must have been a bad rebase or something, I've been checking the commits for the changes each RTBC cycle.

nicxvan’s picture

nicxvan commented
Issue summary: View changes

Cleaning up the IS so the icons work.

The failure is random, it's the theme settings Functional js test.

  • larowlan committed b2a20761 on main 
    task: #3584347 Deprecate and replace system_admin_compact_mode()

By:...
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Version: main » 11.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)
Issue tags: -Needs frontend framework manager review

Committed to main.
Moving to patch to be ported for 11.x backport - doesn't apply cleanly - conflict on system.site.yml

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

Holding off publishing the change record until it is in 11.x

longwave-bot made their first commit to this issue’s fork.

longwave opened merge request !16682

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Patch (to be ported) » Needs review
nicxvan’s picture

nicxvan commented
Status: Needs review » Needs work

Unfortunately there are a bunch of failures, it looks like migrate tests.

mstrelan’s picture

mstrelan commented

I mostly see "MySQL has gone away" errors, let's see if re-running it helps.

nicxvan’s picture

nicxvan commented

I compared the two, the changes are identical, the migration fixtures have the config key that was deprecated so it's throwing an error. I did see some of those other errors too.

nicxvan’s picture

nicxvan commented

It's much fewer, but still legitimate:

  • Drupal\Tests\config_translation\Kernel\Migrate\d6\MigrateSystemSiteTranslationTest::testSystemSite
  • Drupal\Tests\system\Kernel\Migrate\d6\MigrateSystemConfigurationTest::testConfigurationMigration
  • Drupal\Tests\system\Kernel\Migrate\d7\MigrateSystemConfigurationTest::testConfigurationMigration
  • Drupal\Tests\config_translation\Kernel\Migrate\d7\MigrateSystemSiteTranslationTest::testSystemSite

Do we just ignore the deprecations?

nicxvan’s picture

nicxvan commented

Pushed up ignore deprecations for each, not sure if the expect user deprecation is required or not, but I'm not sure it's necessary here since these tests have been removed in main already.

nicxvan’s picture

nicxvan commented
Status: Needs work » Reviewed & tested by the community

I think it's ok for me to RTBC here, it's a bit of a stretch.

My only contribution was adding the IgnoreDeprecations attribute to the four tests that use the deprecated configuration that have already been removed in main: https://git.drupalcode.org/project/drupal/-/merge_requests/16682/diffs?c...

I compared the commit in 55 to the MR in 59 and they were identical.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Reviewed & tested by the community » Needs review

I think we should remove the migration instead of ignoring the tests?

nicxvan’s picture

nicxvan commented
Status: Needs review » Reviewed & tested by the community

That is a better fix!

Thanks for taking care of that.

The backport is exactly the same except it pulls out migrating the admin_compact_mode setting and removes the explicit test for it.

nicxvan’s picture

nicxvan commented
Component: Admin theme » system.module

  • larowlan committed 3e3315d6 on 11.x 
    task: #3584347 Deprecate and replace system_admin_compact_mode()

By:...
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed 3e3315d6037 to 11.x. Thanks!

Published change record

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

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