Skip to content
This repository has been archived by the owner on Mar 9, 2024. It is now read-only.

Remove Accordion Script #205

Merged
merged 9 commits into from
Sep 21, 2023
Merged

Remove Accordion Script #205

merged 9 commits into from
Sep 21, 2023

Conversation

KTS915
Copy link
Member

@KTS915 KTS915 commented Sep 9, 2023

Following PR #197, there is no longer any need for the accordion script. This PR therefore removes it from the JS folder in wp-admin and removes the function calling it from template.php in wp-admin/includes

I have tested this on my own localhost test install. I have also grepped to find all instances of the accordion code being used in the admin, but I have found it only in the template.php file.

Following PR #197, there is no longer any need for the accordion script. This PR therefore removes it from the JS folder in `wp-admin` and removes the function calling it from `template.php` in `wp-admin/includes`
@KTS915 KTS915 changed the title Update template.php Remove Accordion Script Sep 9, 2023
Remove more references to the accordion script
…ript

Confusingly, there are two accordion scripts in the CP admin: one is part of jQuery UI and should not be removed. This commit reverts its removal.
@mattyrob
Copy link
Collaborator

I've done a little testing and the Menu accordions no longer work within Customizer once the dependency is removed:

Screenshot 2023-09-10 at 08 43 20

@KTS915
Copy link
Member Author

KTS915 commented Sep 10, 2023

Good catch, @mattyrob ! I never use the Customizer so never thought of that. I'll see if I can make that work using the details tag instead, like the regular nav-menu, so that we can remove this script.

@KTS915
Copy link
Member Author

KTS915 commented Sep 10, 2023

@mattyrob, I've now taken a look and the menu accordions in the Customizer are working fine for me. Indeed, looking at the script-loader.php file, it's clear that the accordion script isn't used there. The relevant lines are these:

$scripts->add( 'customize-nav-menus', "/wp-admin/js/customize-nav-menus$suffix.js", array( 'jquery', 'wp-backbone', 'customize-controls', 'nav-menu', 'wp-sanitize' ), false, 1 ); $scripts->add( 'customize-preview-nav-menus', "/wp-includes/js/customize-preview-nav-menus$suffix.js", array( 'jquery', 'wp-util', 'customize-preview', 'customize-selective-refresh' ), false, 1 );

No mention of accordion there. I'm wondering if you've removed the wrong accordion version for your test. (As you can see in this PR, I made that mistake myself initially, and removed the one associated with jQuery UI.)

@mattyrob
Copy link
Collaborator

@KTS915 - I'm checked out on this branch and when I get to the screen that I posted a screen grab off before, in Safari and FF, I cannot get the items in the accordion under the "Add Items" to open.

By reverting the changes in commit 06e6037, that section works again.

I should say that anything currently in the Menu seems to work fine.

This replicates the changes made to the regular `nav menus` screen in that it adds the `details` and `summary` tags to the Add Menus section in the Customizer.

This change both improves accessibility and enables the `accordion` script to be removed.
This deletes `display: none;` from the `.accordion-section-content` class and so enables the relevant sections to become visible when toggled using the `details` tag.
@KTS915
Copy link
Member Author

KTS915 commented Sep 10, 2023

@mattyrob Thanks, I managed to replicate the problem and have now pushed some changes to make it work. Specifically, I have added the details and summary tags, as with the regular nav menu, and deleted one line of CSS that would prevent the content of each section from being displayed.

@xxsimoxx
Copy link
Member

I'm getting wrong icon in customizer.

Schermata 2023-09-11 alle 10 19 55

The problem started in #197 but maybe can be fixed in this PR.

@mattyrob
Copy link
Collaborator

@xxsimoxx - I think I may have fixed the icon issue but was working on develop and pushed it by mistake - so I hope it was right.
If not we can revert and do more testing on this PR.

@xxsimoxx
Copy link
Member

@mattyrob the commit in develop fixed the issue.

Copy link
Member

@xxsimoxx xxsimoxx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@mattyrob
Copy link
Collaborator

Is there one more things for this is to consider on this PR, removing src/wp-admin/js/accordion.js from core?

Delete file as it is no longer used
@KTS915
Copy link
Member Author

KTS915 commented Sep 21, 2023

Lol, of course I should have deleted that file! Duh! Now done.

@mattyrob mattyrob merged commit c54f481 into ClassicPress:develop Sep 21, 2023
9 checks passed
@KTS915 KTS915 deleted the Delete-accordion-script branch September 21, 2023 19:34
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants