FIX: Improve topic/header integration when navigating away (#28040)

- Ensure main title is set as 'not visible' when removed from DOM

- `deactivate` -> `willTransition` to ensure proper behavior when navigating between multiple topics

Followup to bdec564d14
This commit is contained in:
David Taylor 2024-07-23 14:57:15 +01:00 committed by GitHub
parent 5b693c61af
commit c333e9d6e6
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
4 changed files with 38 additions and 21 deletions

View File

@ -3,6 +3,7 @@ import { hash } from "@ember/helper";
import { on } from "@ember/modifier";
import { action } from "@ember/object";
import didInsert from "@ember/render-modifiers/modifiers/did-insert";
import willDestroy from "@ember/render-modifiers/modifiers/will-destroy";
import { service } from "@ember/service";
import PluginOutlet from "discourse/components/plugin-outlet";
import { isiPad } from "discourse/lib/utilities";
@ -53,12 +54,26 @@ export default class TopicTitle extends Component {
}
}
@action
handleIntersectionChange(e) {
// Title is in the viewport or below it unusual, but can be caused by
// small viewport and/or large headers. Treat same as if title is on screen.
this.header.mainTopicTitleVisible =
e.isIntersecting || e.boundingClientRect.top > 0;
}
@action
handleTitleDestroy() {
this.header.mainTopicTitleVisible = false;
}
<template>
{{! template-lint-disable no-invalid-interactive }}
<div
{{didInsert this.applyDecorators}}
{{on "keydown" this.keyDown}}
{{observeIntersection this.header.titleIntersectionChanged}}
{{observeIntersection this.handleIntersectionChange}}
{{willDestroy this.handleTitleDestroy}}
id="topic-title"
class="container"
>

View File

@ -52,7 +52,6 @@ export default class TopicFromParams extends DiscourseRoute {
deactivate() {
super.deactivate(...arguments);
this.controllerFor("topic").unsubscribe();
this.header.clearTopic();
}
setupController(controller, params, { _discourse_anchor }) {
@ -122,9 +121,15 @@ export default class TopicFromParams extends DiscourseRoute {
}
@action
willTransition() {
willTransition(transition) {
this.controllerFor("topic").set("previousURL", document.location.pathname);
transition.followRedirects().finally(() => {
if (!this.router.currentRouteName.startsWith("topic.")) {
this.header.clearTopic();
}
});
// NOTE: omitting this return can break the back button when transitioning quickly between
// topics and the latest page.
return true;

View File

@ -1,6 +1,5 @@
import { tracked } from "@glimmer/tracking";
import { registerDestructor } from "@ember/destroyable";
import { action } from "@ember/object";
import { dependentKeyCompat } from "@ember/object/compat";
import Service, { service } from "@ember/service";
import { TrackedMap } from "@ember-compat/tracked-built-ins";
@ -129,22 +128,6 @@ export default class Header extends Service {
return Array.from(buttonsToHide);
}
/**
* Called by a modifier attached to the main topic title element.
*/
@action
titleIntersectionChanged(e) {
if (e.isIntersecting) {
this.mainTopicTitleVisible = true;
} else if (e.boundingClientRect.top > 0) {
// Title is below the curent viewport position. Unusual, but can be caused with
// small viewport and/or large headers. Treat same as if title is on screen.
this.mainTopicTitleVisible = true;
} else {
this.mainTopicTitleVisible = false;
}
}
/**
* Called whenever a topic route is entered. Sets the current topicInfo,
* and makes a guess about whether the main topic title is likely to be visible

View File

@ -262,7 +262,7 @@ RSpec.describe "Glimmer Header", type: :system do
describe "mobile topic-info" do
fab!(:topic)
fab!(:posts) { Fabricate.times(5, :post, topic: topic) }
fab!(:posts) { Fabricate.times(21, :post, topic: topic) }
it "only shows when scrolled down", mobile: true do
visit "/t/#{topic.slug}/#{topic.id}"
@ -286,5 +286,19 @@ RSpec.describe "Glimmer Header", type: :system do
expect(page).not_to have_css("header.d-header .auth-buttons .login-button") # No header buttons
expect(page).to have_css("header.d-header .title-wrapper .topic-link") # Title is shown in header
end
it "shows when jumping from OP to much later post", mobile: true do
visit "/t/#{topic.slug}/#{topic.id}"
expect(page).to have_css("#topic-title") # Main topic title
expect(page).to have_css("header.d-header .auth-buttons .login-button") # header buttons visible when no topic-info in header
page.execute_script("Discourse.visit('/t/#{topic.slug}/#{topic.id}/21');")
expect(page).to have_css("#post_21")
expect(page).not_to have_css("header.d-header .auth-buttons .login-button") # No header buttons
expect(page).to have_css("header.d-header .title-wrapper .topic-link") # Title is shown in header
end
end
end