handleClick(), root state initialization, setting up event listeners, onpopstate(), Drupal Ajax object creation … they would all benefit from being grouped together into a Controller object.

(I realized this thanks to #2713549-4: Refactor onhashchange event handler away: simplicity, consistency, and avoids a whole class of potential problems with JS using hash-based routing.)

CommentFileSizeAuthor
#7 interdiff.txt807 byteswim leers
#7 2713567-7.patch27.22 KBwim leers
#6 interdiff.txt4.89 KBwim leers
#6 2713567-6.patch27.21 KBwim leers
#4 interdiff.txt24 KBwim leers
#4 2713567-4.patch26.03 KBwim leers
#3 2713567-3.patch4.6 KBwim leers

Comments

Wim Leers created an issue. See original summary.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: Refactor all the controller logic into an actual object » Refactor all the controller logic into an actual object: improve understandability/maintainability
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Active » Needs review
StatusFileSize
new 2713567-3.patch4.6 KB

First step.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
StatusFileSize
new 2713567-4.patch26.03 KB
new interdiff.txt24 KB

Split out into:

  • HistoryBasedNavigation
  • ClickBasedNavigation
  • Controller

There is unfortunately one regression: when using the "back" button, the scroll position is no longer being restored… deity knows why :(

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

There is unfortunately one regression: when using the "back" button, the scroll position is no longer being restored… deity knows why :(

Turns out this regression was introduced in #2713549: Refactor onhashchange event handler away: simplicity, consistency, and avoids a whole class of potential problems with JS using hash-based routing! So it'll be up to that issue to fix it.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
StatusFileSize
new 2713567-6.patch27.21 KB
new interdiff.txt4.89 KB

Changes:

  • Document architecture
  • s/HistoryBasedNavigation/HistoryNavigation/
  • s/ClickBasedNavigation/LinkNavigation/
  • Explicitly document that we should get rid of the currentPos global (but not in this issue)
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
StatusFileSize
new 2713567-7.patch27.22 KB
new interdiff.txt807 bytes

Fix eslint errors.

  • Wim Leers committed d2152a7 on 8.x-1.x 
    Issue #2713567 by Wim Leers: Refactor all the controller logic into an...
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

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