Abstract away onNewRoute and routeHasChanged #2359
Conversation
This provides a cleaner interface for dealing with pages that handle multiple routes. We also move `modal` and `drawer` close calls to ensure those happen on route change with the same component.
askvortsov1
requested review from
dsevillamartin,
clarkwinkelmann and
franzliedke
and removed request for
franzliedke
This avoid issues with SettingsPage
| this.currentPath = m.route.get(); | ||
| } | ||
|
|
||
| routeHasChanged() { |
There was a problem hiding this comment.
Do we want this to be public API, or could we just do the test in onbeforeupdate ?
There was a problem hiding this comment.
I think public API is good: subclasses with more complex use cases (like DiscussionPage) could use it to do more of those checks, and also possibly handle some side effects if the route hasn't changed but we still want to do something (like scrolling to a post).
| /** | ||
| * A class name to apply to the body while the route is active. | ||
| * | ||
| * @type {String} | ||
| */ | ||
| this.bodyClass = ''; | ||
|
|
||
| this.currentPath = m.route.get(); |
There was a problem hiding this comment.
This line can be probably removed I think, since it's called in onNewRoute already.
| @@ -12,15 +12,18 @@ export default class Page extends Component { | |||
|
|
|||
| this.onNewRoute(); | |||
There was a problem hiding this comment.
I'd move that to the end of the function. This way everything in oninit would run before onNewRoute.
| // Otherwise, load in a new discussion. The `else` branch will | ||
| // be followed when `onNewRoute` is called from `oninit`. | ||
| const idParam = m.route.param('id'); | ||
| if (this.discussion && idParam && idParam.split('-')[0] === this.discussion.id()) { |
There was a problem hiding this comment.
I'd split this out in this PR already, since the diff is very hard to read. Something like this:
isSameRoute() {
const idParam = m.route.param('id');
return this.discussion && idParam && idParam.split('-')[0] === this.discussion.id();
}
onSameRoute() {
const near = m.route.param('near') || '1';
if (near !== String(this.near)) {
this.stream.goToNumber(near);
}
this.near = near;
}and:
onNewRoute() {
if (this.isSameRoute()) return this.onSameRoute();
super.onNewRoute();
....
}
|
Obsolete with #2378 |
This provides a cleaner interface for dealing with pages that handle multiple routes. We also move
modalanddrawerclose calls to ensure those happen on route change with the same component.