From b8e9d119a76da7a79474a22245a99ea8dd0b78cd Mon Sep 17 00:00:00 2001 From: Tobias Dillmann Date: Wed, 25 Sep 2024 13:42:33 +0200 Subject: [PATCH 1/6] [#57904] meeting reload takes you back to your previous position --- .../poll-for-changes.controller.ts | 33 +++++++++++++++++++ .../meetings/update_flash_component.rb | 2 +- 2 files changed, 34 insertions(+), 1 deletion(-) diff --git a/frontend/src/stimulus/controllers/poll-for-changes.controller.ts b/frontend/src/stimulus/controllers/poll-for-changes.controller.ts index b7b9407b5823..0fc01cc7dd78 100644 --- a/frontend/src/stimulus/controllers/poll-for-changes.controller.ts +++ b/frontend/src/stimulus/controllers/poll-for-changes.controller.ts @@ -52,6 +52,9 @@ export default class PollForChangesController extends ApplicationController { void this.triggerTurboStream(); }, this.intervalValue || 10_000); } + + window.addEventListener('beforeunload', this.rememberCurrentScrollPosition.bind(this)); + window.addEventListener('DOMContentLoaded', this.autoscrollToLastKnownPosition.bind(this)); } disconnect() { @@ -73,4 +76,34 @@ export default class PollForChangesController extends ApplicationController { } }); } + + rememberCurrentScrollPosition() { + const currentPosition = document.getElementById('content-body')?.scrollTop; + + if (currentPosition !== undefined) { + sessionStorage.setItem(this.scrollPositionKey(), currentPosition.toString()); + } + } + + autoscrollToLastKnownPosition() { + const params = new URLSearchParams(window.location.search); + if (params.get('autoscroll') !== 'true') { + return; + } + + const lastKnownPos = sessionStorage.getItem(this.scrollPositionKey()); + if (lastKnownPos) { + const content = document.getElementById('content-body'); + + if (content) { + content.scrollTop = parseInt(lastKnownPos, 10); + } + } + + sessionStorage.removeItem(this.scrollPositionKey()); + } + + private scrollPositionKey():string { + return `${this.urlValue}/scrollPosition`; + } } diff --git a/modules/meeting/app/components/meetings/update_flash_component.rb b/modules/meeting/app/components/meetings/update_flash_component.rb index 31c991ed513f..ad1996463f74 100644 --- a/modules/meeting/app/components/meetings/update_flash_component.rb +++ b/modules/meeting/app/components/meetings/update_flash_component.rb @@ -42,7 +42,7 @@ def call ) do |banner| banner.with_action_button( tag: :a, - href: helpers.meeting_path(meeting), + href: helpers.meeting_path(meeting, autoscroll: true), data: { turbo: false }, size: :medium ) { I18n.t("label_meeting_reload") } From dc7470d99c5e92d291f262ce7492814446fdf5fb Mon Sep 17 00:00:00 2001 From: Tobias Dillmann Date: Wed, 25 Sep 2024 15:21:11 +0200 Subject: [PATCH 2/6] [#57904] make autoscroll configurable by controller param --- .../stimulus/controllers/poll-for-changes.controller.ts | 8 ++++++-- .../app/components/meetings/header_component.html.erb | 3 ++- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/frontend/src/stimulus/controllers/poll-for-changes.controller.ts b/frontend/src/stimulus/controllers/poll-for-changes.controller.ts index 0fc01cc7dd78..fb49f49f888e 100644 --- a/frontend/src/stimulus/controllers/poll-for-changes.controller.ts +++ b/frontend/src/stimulus/controllers/poll-for-changes.controller.ts @@ -36,11 +36,13 @@ export default class PollForChangesController extends ApplicationController { url: String, interval: Number, reference: String, + autoscrollEnabled: Boolean, }; declare referenceValue:string; declare urlValue:string; declare intervalValue:number; + declare autoscrollEnabledValue:boolean; private interval:number; @@ -53,8 +55,10 @@ export default class PollForChangesController extends ApplicationController { }, this.intervalValue || 10_000); } - window.addEventListener('beforeunload', this.rememberCurrentScrollPosition.bind(this)); - window.addEventListener('DOMContentLoaded', this.autoscrollToLastKnownPosition.bind(this)); + if (this.autoscrollEnabledValue) { + window.addEventListener('beforeunload', this.rememberCurrentScrollPosition.bind(this)); + window.addEventListener('DOMContentLoaded', this.autoscrollToLastKnownPosition.bind(this)); + } } disconnect() { diff --git a/modules/meeting/app/components/meetings/header_component.html.erb b/modules/meeting/app/components/meetings/header_component.html.erb index 13a451beba3b..7fdcbb846635 100644 --- a/modules/meeting/app/components/meetings/header_component.html.erb +++ b/modules/meeting/app/components/meetings/header_component.html.erb @@ -3,7 +3,8 @@ controller: "poll-for-changes", poll_for_changes_reference_value: @meeting.changed_hash, poll_for_changes_url_value: check_for_updates_meeting_path(@meeting), - poll_for_changes_interval_value: check_for_updates_interval + poll_for_changes_interval_value: check_for_updates_interval, + poll_for_changes_autoscroll_enabled_value: true } component_wrapper do From d3fc2ae398e78db971bd796e0f23b704463941dc Mon Sep 17 00:00:00 2001 From: Tobias Dillmann Date: Wed, 25 Sep 2024 16:05:57 +0200 Subject: [PATCH 3/6] [#57904] move content controller further out MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This way, a page controller can grasp more DOM elements on the page. Co-authored-by: Oliver Günther --- app/views/layouts/base.html.erb | 7 +++---- docs/development/concepts/stimulus/README.md | 2 +- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/app/views/layouts/base.html.erb b/app/views/layouts/base.html.erb index 3477e188cbc4..b30d04035b06 100644 --- a/app/views/layouts/base.html.erb +++ b/app/views/layouts/base.html.erb @@ -116,7 +116,7 @@ See COPYRIGHT and LICENSE files for more details. <% end %>
-
+ <%= content_tag :main, id: "content-wrapper", class: initial_classes, data: stimulus_content_data do %> <%# Primerized flash messages are being rendered separately %>
<%= render_primerized_flash %> @@ -134,8 +134,7 @@ See COPYRIGHT and LICENSE files for more details. <% end %> <%= content_tag :div, id: 'content', - class: "#{initial_classes} #{'content--split' if content_for?(:content_body_right)}", - data: stimulus_content_data do %> + class: "#{initial_classes} #{'content--split' if content_for?(:content_body_right)}" do %>

<%= t(:label_content) %>

<% if content_for?(:content_header) %>
@@ -162,7 +161,7 @@ See COPYRIGHT and LICENSE files for more details. <%= content_for :content_body_right %> <% end %> <% end %> -
+ <% end %> diff --git a/docs/development/concepts/stimulus/README.md b/docs/development/concepts/stimulus/README.md index af7199d220ff..5bb9b78b1c50 100644 --- a/docs/development/concepts/stimulus/README.md +++ b/docs/development/concepts/stimulus/README.md @@ -44,7 +44,7 @@ You need to take care to prefix all actions, values etc. with the exact same pat ### Requiring a page controller -If you have a single controller used in a partial, we have added a helper to use in a partial in order to append a controller to the `#content`tag. This is useful if your template doesn't have a single DOM root. For example, to load the dynamic `project-storage-form` controller and provide a custom value to it: +If you have a single controller used in a partial, we have added a helper to use in a partial in order to append a controller to the `#content-wrapper` tag. This is useful if your template doesn't have a single DOM root. For example, to load the dynamic `project-storage-form` controller and provide a custom value to it: ```erb <% content_controller 'project-storage-form', From 58f8a1fd4bba07dda9bb8c3b2359a0c6bd0cfc97 Mon Sep 17 00:00:00 2001 From: Tobias Dillmann Date: Wed, 25 Sep 2024 16:26:25 +0200 Subject: [PATCH 4/6] [#57904] use targetConnected event in Stimulus --- .../controllers/poll-for-changes.controller.ts | 14 ++++++++------ .../meetings/header_component.html.erb | 17 +++++++---------- .../meetings/update_flash_component.rb | 4 ++-- 3 files changed, 17 insertions(+), 18 deletions(-) diff --git a/frontend/src/stimulus/controllers/poll-for-changes.controller.ts b/frontend/src/stimulus/controllers/poll-for-changes.controller.ts index fb49f49f888e..811dc91926fd 100644 --- a/frontend/src/stimulus/controllers/poll-for-changes.controller.ts +++ b/frontend/src/stimulus/controllers/poll-for-changes.controller.ts @@ -39,6 +39,10 @@ export default class PollForChangesController extends ApplicationController { autoscrollEnabled: Boolean, }; + static targets = ['reloadButton']; + + declare reloadButtonTarget:HTMLLinkElement; + declare referenceValue:string; declare urlValue:string; declare intervalValue:number; @@ -56,7 +60,6 @@ export default class PollForChangesController extends ApplicationController { } if (this.autoscrollEnabledValue) { - window.addEventListener('beforeunload', this.rememberCurrentScrollPosition.bind(this)); window.addEventListener('DOMContentLoaded', this.autoscrollToLastKnownPosition.bind(this)); } } @@ -66,6 +69,10 @@ export default class PollForChangesController extends ApplicationController { clearInterval(this.interval); } + reloadButtonTargetConnected() { + this.reloadButtonTarget.addEventListener('click', this.rememberCurrentScrollPosition.bind(this)); + } + triggerTurboStream() { void fetch(`${this.urlValue}?reference=${this.referenceValue}`, { headers: { @@ -90,11 +97,6 @@ export default class PollForChangesController extends ApplicationController { } autoscrollToLastKnownPosition() { - const params = new URLSearchParams(window.location.search); - if (params.get('autoscroll') !== 'true') { - return; - } - const lastKnownPos = sessionStorage.getItem(this.scrollPositionKey()); if (lastKnownPos) { const content = document.getElementById('content-body'); diff --git a/modules/meeting/app/components/meetings/header_component.html.erb b/modules/meeting/app/components/meetings/header_component.html.erb index 7fdcbb846635..01f201391234 100644 --- a/modules/meeting/app/components/meetings/header_component.html.erb +++ b/modules/meeting/app/components/meetings/header_component.html.erb @@ -1,17 +1,14 @@ -<%= - params = { - controller: "poll-for-changes", - poll_for_changes_reference_value: @meeting.changed_hash, - poll_for_changes_url_value: check_for_updates_meeting_path(@meeting), - poll_for_changes_interval_value: check_for_updates_interval, - poll_for_changes_autoscroll_enabled_value: true - } +<% + helpers.content_controller "poll-for-changes", + poll_for_changes_reference_value: @meeting.changed_hash, + poll_for_changes_url_value: check_for_updates_meeting_path(@meeting), + poll_for_changes_interval_value: check_for_updates_interval, + poll_for_changes_autoscroll_enabled_value: true component_wrapper do render(Primer::OpenProject::PageHeader.new( test_selector: "meeting-page-header", - state: @state, - data: params + state: @state )) do |header| header.with_title do |title| title.with_editable_form(model: @meeting, diff --git a/modules/meeting/app/components/meetings/update_flash_component.rb b/modules/meeting/app/components/meetings/update_flash_component.rb index ad1996463f74..0b25370a52e3 100644 --- a/modules/meeting/app/components/meetings/update_flash_component.rb +++ b/modules/meeting/app/components/meetings/update_flash_component.rb @@ -42,8 +42,8 @@ def call ) do |banner| banner.with_action_button( tag: :a, - href: helpers.meeting_path(meeting, autoscroll: true), - data: { turbo: false }, + href: helpers.meeting_path(meeting), + data: { turbo: false, poll_for_changes_target: "reloadButton" }, size: :medium ) { I18n.t("label_meeting_reload") } From 9598aa73de00a3972210994a709f2a21f8868005 Mon Sep 17 00:00:00 2001 From: Tobias Dillmann Date: Wed, 25 Sep 2024 16:56:51 +0200 Subject: [PATCH 5/6] [#57904] render the header --- .../meeting/app/components/meetings/header_component.html.erb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/meeting/app/components/meetings/header_component.html.erb b/modules/meeting/app/components/meetings/header_component.html.erb index 01f201391234..c27700bbe91c 100644 --- a/modules/meeting/app/components/meetings/header_component.html.erb +++ b/modules/meeting/app/components/meetings/header_component.html.erb @@ -1,4 +1,4 @@ -<% +<%= helpers.content_controller "poll-for-changes", poll_for_changes_reference_value: @meeting.changed_hash, poll_for_changes_url_value: check_for_updates_meeting_path(@meeting), From 169a00418e0e18f03b9c71bcabb9c988ac31078b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oliver=20G=C3=BCnther?= Date: Wed, 25 Sep 2024 17:14:01 +0200 Subject: [PATCH 6/6] Target controller on correct dom --- modules/meeting/spec/support/pages/structured_meeting/show.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/meeting/spec/support/pages/structured_meeting/show.rb b/modules/meeting/spec/support/pages/structured_meeting/show.rb index 10a9367420d4..34e531761a98 100644 --- a/modules/meeting/spec/support/pages/structured_meeting/show.rb +++ b/modules/meeting/spec/support/pages/structured_meeting/show.rb @@ -43,7 +43,7 @@ def trigger_dropdown_menu_item(name) def trigger_change_poll script = <<~JS - var target = document.querySelector('[data-test-selector="meeting-page-header"]'); + var target = document.querySelector('#content-wrapper'); var controller = window.Stimulus.getControllerForElementAndIdentifier(target, 'poll-for-changes') controller.triggerTurboStream(); JS