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

Button for Navigation Reorder is not working correctly #20616

Closed
jcastro-dotcms opened this issue Jul 1, 2021 · 9 comments · Fixed by #20716
Closed

Button for Navigation Reorder is not working correctly #20616

jcastro-dotcms opened this issue Jul 1, 2021 · 9 comments · Fixed by #20716

Comments

@jcastro-dotcms
Copy link
Contributor

jcastro-dotcms commented Jul 1, 2021

Reported via Support: https://dotcms.zendesk.com/agent/tickets/104561
Reproducible in dotCMS 5.3.8.4, 5.3.8.5 up to latest master (21.05.1)

According to our official documentation: https://dotcms.com/docs/latest/navtool-viewtool#ExampleAddReorderButton , users can display the Reorder Button for the navigation items. In previous versions of dotCMS, the Reorder feature automatically detected the folder path of the page it was placed in, and display the appropriate children up to two levels deep.

However, I was able to confirm with @dsilvam that this feature is not working as expected in more recent versions of dotCMS:

  1. By default, the Reorder Button is always showing the level-1 folders of the Site, with no regards to the location of the page that is displaying it. This was probably introduced by the following code refactoring in dotCMS 5.1.0: 17e924e

  2. When moving folders around in the Reorder window, the following JavaScript error is being logged to the browser's console dozens of times per second:

Uncaught InternalError: too much recursion

And the UI will not let you make any other change anymore. You have to close the Reorder window and open it up again.

  1. Moving navigation items around that are further down the list is presenting problems because the placeholder is not being recognized, i.e., the drag-and-drop is not showing up. This means that navigation lists with many items can only be re-ordered partially.

To Reproduce
For concrete examples and data source, please ask me.

Expected behavior
The Reorder Button should work correctly the same way it does in previous dotCMS versions.

@jcastro-dotcms
Copy link
Contributor Author

The Velocity code that must be used to render the Reorder Buttons is the following:

## ADD REORDER MENU BUTTON
#if ($EDIT_MODE && $PUBLISH_HTMLPAGE_PERMISSION)
   <div class="reorder-menu">
       #set($_formId="dot_form_menu_$date.date.time")
       #set ($folder = $folderAPI.findCurrentFolder($startFromPath, $host))
       #set ($menuId = $folder.inode)       
       #orderMenu()
   </div>
#end

The example code in our official documentation: https://dotcms.com/docs/latest/navtool-viewtool#ExampleAddReorderButton seems to be outdated, or not working anymore.

@dsilvam
Copy link
Contributor

dsilvam commented Jul 27, 2021

This PR #20716 fixes sending the proper start level for the reordering , solving point 1.

For point 2, there are two options: downgrading prototype from 1.7.3 to 1.5.0, which is the version that comes with the scriptaculous we use; or update scriptaculous to version 1.9.0 which works with prototype 1.7. That solves point 2, but need to be careful if this breaks something else. However, even after fixing the infinite recursion issue the drag and drop is not properly working, not letting the drop to happen. This might have happened after converting the reorder to a popup here 17e924e.

@jcastro-dotcms jcastro-dotcms added the LTS: Discuss To be discussed between Support & R&D label Jul 27, 2021
@wezell
Copy link
Contributor

wezell commented Jul 27, 2021

Reordering works the first time and not the second.

@dsilvam dsilvam assigned alfredo-dotcms and unassigned dsilvam Jul 27, 2021
dsilvam added a commit that referenced this issue Jul 29, 2021
* #20616 Send proper startLevel

* Upgraded Scriptaculous and style fixes on menu reorder

Co-authored-by: Alfredo Li <alfredo@dotcms.com>
@dsilvam dsilvam linked a pull request Jul 29, 2021 that will close this issue
@dsilvam dsilvam removed their assignment Jul 29, 2021
@jcastro-dotcms jcastro-dotcms added LTS : Next Ticket that will be added to LTS and removed LTS: Discuss To be discussed between Support & R&D Severity : Support Priority labels Aug 3, 2021
@nollymar
Copy link
Contributor

nollymar commented Aug 5, 2021

Internal QA: The issues commented on the description are fixed, however, the reorder changes are not being persisted:

Screen.Recording.2021-08-05.at.11.38.30.AM.mov

@dsilvam dsilvam removed their assignment Aug 6, 2021
@alfredo-dotcms
Copy link
Contributor

After some testing, seems like the error happens on the About page, for some reason an error is being thrown on the BE.

If you do this on other page with a smaller menu, it works!
https://monosnap.com/file/9OuT43vbPccXTDwPUdGInGX5I2PSZQ

the reordering on the FE is working
https://monosnap.com/file/V54JmqgsdNH1oIrMjWaWWBtWc5JWkd

@nollymar nollymar modified the milestones: Maintenance Sprint, Falcon Current Aug 24, 2021
@dsilvam
Copy link
Contributor

dsilvam commented Aug 24, 2021

Passed Internal QA.
To test against given dataset, which has assets with old auto-increment ids, is needed to set dotshortyapi_use_legacy=true in dotmarketing props

@dsilvam dsilvam removed their assignment Aug 24, 2021
@bryanboza
Copy link
Member

Fixed, tested on release-21.09 // Postgres // FF

@dsilvam
Copy link
Contributor

dsilvam commented Aug 25, 2021

@bryanboza I suggest testing this one with a custom dataset we have. Please contact me or @jcastro-dotcms

@wezell wezell closed this as completed Aug 25, 2021
@swicken-dotcms swicken-dotcms added Release : 21.06.4 Included in LTS patch release 21.06.4 Release : 5.3.8.6 Included in LTS patch release 5.3.8.6 and removed LTS : Next Ticket that will be added to LTS Severity : CS Priority labels Nov 19, 2021
@jcastro-dotcms jcastro-dotcms added Release : 5.3.8.6.1 Included in LTS patch release 5.3.8.6.1 Release : 5.3.8.7 Included in LTS patch release 5.3.8.7 and removed Release : 5.3.8.6 Included in LTS patch release 5.3.8.6 labels Nov 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants