Skip to content

Conversation

w-4
Copy link
Contributor

@w-4 w-4 commented Oct 10, 2020

Fixes the following issue:

  1. Go to https://nightly.flarum.site
  2. Click the home link
  3. It doesn't refresh the page, when already on the '/' route.

Depends on #2378. Going to rebase after it's merged.

Changes proposed in this pull request:
Use the route resolver onmatch method to check if links have requested a forced refresh.

Reviewers should focus on:
Do we want to make the force refresh the default for links created with the Link component?

Confirmed

  • Frontend changes: tested on a local Flarum installation.

@w-4 w-4 mentioned this pull request Oct 10, 2020
1 task
@w-4
Copy link
Contributor Author

w-4 commented Oct 10, 2020

Regarding the links here are the options:

  1. Use the Link component force attribute, so that clicking on a link twice will both times fully re-render the page, otherwise it will just diff the page (when the URL is the same). We can then decide on a case by case basis.
  2. Or make the above force behavior the default for all Links. This was the case in beta.13.

When setting routes programmatically, the default behavior will be to just diff, if the URL is identical. One can use the setRouteWithForcedRefresh helper instead of m.route.set to make the page fully re-render.

@askvortsov1
Copy link
Member

I prefer option 1. With the changes in #2378, that behavior is recovered where relevant. The only change I'd make is for LinkButtons to have force enabled.

Also, when trying out #2378, setRouteWithForcedRefresh seemed to work. I'm not sure if the changes are necessary, although I do like that the logic for key generation will be fully contained in the route resolver. I don't like app.forceRefresh though (preferable to avoid introducing new globals where possible imo): couldn't we modify the options passed into m.route.set like with the options in the Link component?

@w-4
Copy link
Contributor Author

w-4 commented Oct 16, 2020

Let's close this, it seems to work now more or less. The only thing that's weird is that the site refreshes for me on https://nightly.flarum.site, but not when I run it locally.

@w-4 w-4 closed this Oct 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants