Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FieldOptions with isOrderable doesn't maintain order #10529

Closed
NateWr opened this issue Oct 18, 2024 · 4 comments
Closed

FieldOptions with isOrderable doesn't maintain order #10529

NateWr opened this issue Oct 18, 2024 · 4 comments
Assignees
Labels
Bug:2:Moderate A bug that is causing problems for a substantial minority of users. UI/UX Issues affecting the user interface/user experience

Comments

@NateWr
Copy link
Contributor

NateWr commented Oct 18, 2024

Describe the bug
When a <FieldOptions> component has the isOrderable prop, the options can be reordered. However, when loading the field, the options are not displayed in the order they were last saved in.

The only place an orderable FieldOptions is used is Settings > Website > Appearance > Setup > Sidebar. There is a workaround on the backend to reorder the options before they are sent to the Vue component.

foreach ($currentBlocks as $plugin) {
if (isset($plugins[$plugin])) {
$enabledOptions[] = [
'value' => $plugin,
'label' => htmlspecialchars($plugins[$plugin]->getDisplayName()),
];
}
}
foreach ($plugins as $pluginName => $plugin) {
if (!in_array($pluginName, $currentBlocks)) {
$disabledOptions[] = [
'value' => $pluginName,
'label' => htmlspecialchars($plugin->getDisplayName()),
];
}
}
$sidebarOptions = array_merge($enabledOptions, $disabledOptions);

To Reproduce
The easiest way to reproduce this is to remove the workaround code in PKPAppearanceSetupForm.php and then reproduce it with the sidebar setting:

  1. Apply the following diff to main:
diff --git a/classes/components/forms/context/PKPAppearanceSetupForm.php b/classes/components/forms/context/PKPAppearanceSetupForm.php
index 84c05c16c7..684d037c1c 100644
--- a/classes/components/forms/context/PKPAppearanceSetupForm.php
+++ b/classes/components/forms/context/PKPAppearanceSetupForm.php
@@ -43,33 +43,18 @@ class PKPAppearanceSetupForm extends FormComponent
         $this->action = $action;
         $this->locales = $locales;
         $sidebarOptions = [];
-        $enabledOptions = [];
-        $disabledOptions = [];
 
         $currentBlocks = (array) $context->getData('sidebar');
 
         $plugins = PluginRegistry::loadCategory('blocks', true);
 
-        foreach ($currentBlocks as $plugin) {
-            if (isset($plugins[$plugin])) {
-                $enabledOptions[] = [
-                    'value' => $plugin,
-                    'label' => htmlspecialchars($plugins[$plugin]->getDisplayName()),
-                ];
-            }
-        }
-
         foreach ($plugins as $pluginName => $plugin) {
-            if (!in_array($pluginName, $currentBlocks)) {
-                $disabledOptions[] = [
-                    'value' => $pluginName,
-                    'label' => htmlspecialchars($plugin->getDisplayName()),
-                ];
-            }
+            $sidebarOptions[] = [
+                'value' => $pluginName,
+                'label' => htmlspecialchars($plugin->getDisplayName()),
+            ];
         }
 
-        $sidebarOptions = array_merge($enabledOptions, $disabledOptions);
-
         $this->addField(new FieldUploadImage('pageHeaderLogoImage', [
             'label' => __('manager.setup.logo'),
             'value' => $context->getData('pageHeaderLogoImage'),
  1. Go to Settings > Website > Appearance > Setup > Sidebar.
  2. Enable one of the sidebar blocks near the bottom of the list and move it to the top.
  3. Save the form.
  4. Reload the page.
  5. See the enabled sidebar block is at the bottom again.

What application are you using?
This effects as far back as stable-3_3_0 and probably earlier.

Additional information
This bug is causing problems for me when using the field as a theme option. I tried using a similar backend workaround, but it causes serious side effects. In short, I must fetch the value of the theme option to sort the order of the options. However, once I fetch the value of the theme option, all theme options are cached. This means the resorting has to be done after all theme options are defined. I can do this, but it would break any theme options defined in a child theme.

NateWr added a commit to NateWr/ui-library that referenced this issue Oct 18, 2024
NateWr added a commit to NateWr/pkp-lib that referenced this issue Oct 18, 2024
@NateWr
Copy link
Contributor Author

NateWr commented Oct 18, 2024

The following PR fixes the issue in FieldOptions.vue by sorting the options in the mounted hook. It also removes the workaround code from PKPAppearanceSetupForm.php.

I opened the PR against main because I'm having trouble running npm run [build | start] in my stable-3_3_0 checkout. I believe it may be a node version issue. Ideally, this would be back-ported to stable-3_3_0.

PRs:

main:
pkp/ui-library#429 (merged)
#10530 (merged)

stable-3_4_0:
pkp/ui-library#435
#10564

stable-3_3_0:
pkp/ui-library#436
#10565


TESTS ONLY:

main:
pkp/ojs#4475 (closed)

stable-3_4_0:
pkp/ojs#4489
pkp/omp#1739
pkp/ops#797

stable-3_3_0:
pkp/ojs#4490
pkp/omp#1740
pkp/ops#798

@jardakotesovec jardakotesovec self-assigned this Oct 21, 2024
@jardakotesovec jardakotesovec added Bug:2:Moderate A bug that is causing problems for a substantial minority of users. UI/UX Issues affecting the user interface/user experience labels Oct 21, 2024
blesildaramirez pushed a commit to pkp/ui-library that referenced this issue Oct 24, 2024
blesildaramirez added a commit to blesildaramirez/ui-library that referenced this issue Oct 24, 2024
blesildaramirez added a commit to blesildaramirez/pkp-lib that referenced this issue Oct 24, 2024
blesildaramirez added a commit to blesildaramirez/ojs that referenced this issue Oct 24, 2024
blesildaramirez added a commit to blesildaramirez/ui-library that referenced this issue Oct 24, 2024
blesildaramirez added a commit to blesildaramirez/pkp-lib that referenced this issue Oct 24, 2024
blesildaramirez added a commit to blesildaramirez/ojs that referenced this issue Oct 24, 2024
blesildaramirez added a commit to blesildaramirez/omp that referenced this issue Oct 24, 2024
blesildaramirez added a commit to blesildaramirez/ops that referenced this issue Oct 24, 2024
blesildaramirez added a commit to blesildaramirez/omp that referenced this issue Oct 24, 2024
blesildaramirez added a commit to blesildaramirez/ops that referenced this issue Oct 24, 2024
@NateWr
Copy link
Contributor Author

NateWr commented Nov 1, 2024

@blesildaramirez just following up on this. It looks like maybe the PRs to main were merged but not the ones to the stable branch.

@jardakotesovec
Copy link
Contributor

@NateWr Should be now all merged and testing PRs closed. Can you double check that and close the issue? Thanks!

@NateWr
Copy link
Contributor Author

NateWr commented Nov 12, 2024

Confirmed working in stable-3_3_0. Looks like the other PRs were all merged. Thanks!

@NateWr NateWr closed this as completed Nov 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug:2:Moderate A bug that is causing problems for a substantial minority of users. UI/UX Issues affecting the user interface/user experience
Projects
None yet
Development

No branches or pull requests

3 participants