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.

Sign up for GitHub

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

Jump to bottom

Abstract away onNewRoute and routeHasChanged #2359

Closed
wants to merge 3 commits into from
Closed

Abstract away onNewRoute and routeHasChanged #2359

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
46cb320
Abstract away onNewRoute and routeHasChanged
askvortsov1 Oct 5, 2020
759eb80
Remove redundant code from IndexPage oninit
askvortsov1 Oct 5, 2020
d17cc1b
Only automatically load page for UserPage if username param is present
askvortsov1 Oct 5, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 19 additions & 3 deletions js/src/common/components/Page.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,15 +12,18 @@ export default class Page extends Component {

this.onNewRoute();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd move that to the end of the function. This way everything in oninit would run before onNewRoute.


app.drawer.hide();
app.modal.close();

/**
* A class name to apply to the body while the route is active.
*
* @type {String}
*/
this.bodyClass = '';

this.currentPath = m.route.get();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line can be probably removed I think, since it's called in onNewRoute already.

}

routeHasChanged() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want this to be public API, or could we just do the test in onbeforeupdate ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

return this.currentPath !== m.route.get();
}

/**
Expand All @@ -30,8 +33,21 @@ export default class Page extends Component {
* adjust the current route name.
*/
onNewRoute() {
this.currentPath = m.route.get();

app.previous = app.current;
app.current = new PageState(this.constructor, { routeName: this.attrs.routeName });

app.drawer.hide();
app.modal.close();
}

onbeforeupdate(vnode, old) {
super.onbeforeupdate(vnode, old);

if (this.routeHasChanged()) {
this.onNewRoute();
}
}

oncreate(vnode) {
Expand Down
56 changes: 22 additions & 34 deletions js/src/forum/components/DiscussionPage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,23 +32,9 @@ export default class DiscussionPage extends Page {
*/
this.near = m.route.param('near') || 0;

this.load();

// If the discussion list has been loaded, then we'll enable the pane (and
// hide it by default). Also, if we've just come from another discussion
// page, then we don't want Mithril to redraw the whole page – if it did,
// then the pane would redraw which would be slow and would cause problems with
// event handlers.
if (app.discussions.hasDiscussions()) {
app.pane.enable();
app.pane.hide();
}

app.history.push('discussion');

this.bodyClass = 'App--discussion';

this.prevRoute = m.route.get();
}

onremove() {
Expand Down Expand Up @@ -95,30 +81,32 @@ export default class DiscussionPage extends Page {
);
}

onbeforeupdate(vnode) {
super.onbeforeupdate(vnode);

if (m.route.get() !== this.prevRoute) {
this.onNewRoute();
this.prevRoute = m.route.get();
onNewRoute() {
// If we have routed to the same discussion as we were viewing previously,
// prompt the post stream to jump to the new 'near' param.
// 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()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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();

    ....

}

const near = m.route.param('near') || '1';

if (near !== String(this.near)) {
this.stream.goToNumber(near);
}

// If we have routed to the same discussion as we were viewing previously,
// cancel the unloading of this controller and instead prompt the post
// stream to jump to the new 'near' param.
if (this.discussion) {
const idParam = m.route.param('id');
this.near = near;
} else {
super.onNewRoute();

if (idParam && idParam.split('-')[0] === this.discussion.id()) {
const near = m.route.param('near') || '1';
this.discussion = null;

if (near !== String(this.near)) {
this.stream.goToNumber(near);
}
this.load();

this.near = near;
} else {
this.oninit(vnode);
}
// If the discussion list has been loaded, then we'll enable the pane (and
// hide it by default).
if (app.discussions.hasDiscussions()) {
app.pane.enable();
app.pane.hide();
}
}
}
Expand Down
32 changes: 9 additions & 23 deletions js/src/forum/components/IndexPage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,14 @@ export default class IndexPage extends Page {
this.lastDiscussion = app.previous.get('discussion');
}

app.history.push('index', app.translator.trans('core.forum.header.back_to_index_tooltip'));

this.bodyClass = 'App--index';
}

onNewRoute() {
super.onNewRoute();

// If the user is coming from the discussion list, then they have either
// just switched one of the parameters (filter, sort, search) or they
// probably want to refresh the results. We will clear the discussion list
Expand All @@ -39,29 +47,7 @@ export default class IndexPage extends Page {

app.discussions.refreshParams(app.search.params());

app.history.push('index', app.translator.trans('core.forum.header.back_to_index_tooltip'));

this.bodyClass = 'App--index';

this.currentPath = m.route.get();
}

onbeforeupdate(vnode) {
super.onbeforeupdate(vnode);

const curPath = m.route.get();

if (this.currentPath !== curPath) {
this.onNewRoute();

app.discussions.clear();

app.discussions.refreshParams(app.search.params());

this.currentPath = curPath;

this.setTitle();
}
this.setTitle();
}

view() {
Expand Down
13 changes: 4 additions & 9 deletions js/src/forum/components/UserPage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,18 +27,13 @@ export default class UserPage extends Page {
this.user = null;

this.bodyClass = 'App--user';

this.prevUsername = m.route.param('username');
}

onbeforeupdate() {
const currUsername = m.route.param('username');
if (currUsername !== this.prevUsername) {
this.onNewRoute();

this.prevUsername = currUsername;
onNewRoute() {
super.onNewRoute();

this.loadUser(currUsername);
if (m.route.param('username')) {
this.loadUser(m.route.param('username'));
}
}

Expand Down