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.

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

Fix dashboard not applying mounting point when linking or redirecting to another page #1067

Closed
wants to merge 2 commits into from

Conversation

cyrilchandelier
Copy link
Contributor

TL;DR: single app redirection is not the root of our issue here, all mounted dashboards seem broken when using 1.3.0.

When upgrading to the latest version of the dashboard, I encountered the same issue as folks in #1054: app redirects and doesn't keep the mounting point.

Since I made the single app redirection changes in the first place (#958) and it was just a couple lines of code, I started investigating and I'm now 100% confident the problem was misattributed to the single app redirection in the first place.

Unless I'm missing something, something changed between 1.2.0 and 1.3.0. Commenting the code I wrote in #958 didn't fix the issue. I tracked the bad redirection to the following line:

// Dashboard.js#312
<Redirect from='/' to='/apps' />`

But that's not it! Basically, it looks like the mounting point is lost and the navigation is totally broken when the mountPath is not /. I've no idea how it was even working before since the whole app is referring/redirecting to /apps everywhere, without referencing the eventual mount path.

This PR is the result of a painful work trying to add the mountPath everywhere I found it was needed, but to be honest, I had no idea was I was doing the whole time, there might be a simple solution.

@flovilmart: I would love your assistance and opinion on this, thanks!

@flovilmart
Copy link
Contributor

I believe the problem is somewhere else and that rewriting it all was not necessary. I may be wrong. I don’t maintain this project anymore, so check with @acinader, he may be able to help

Sent with GitHawk

@cyrilchandelier cyrilchandelier requested review from acinader and removed request for flovilmart April 30, 2019 17:12
@acinader acinader requested a review from dplewis May 2, 2019 17:43
@acinader
Copy link
Contributor

acinader commented May 2, 2019

@dplewis would you be willing to look at this? I don't have the problem...

@dplewis
Copy link
Member

dplewis commented May 2, 2019

@acinader Just looked into it. A lot of dependencies were updated. The problem is in the history package. You have to set a basename (base url / mountpath) to fix this issue.

I can do a separate PR with a sample server for testing.

@cyrilchandelier Thanks for getting started on this, I know it must have taken a lot of time.

@dplewis
Copy link
Member

dplewis commented May 3, 2019

Superced via #1070

@dplewis dplewis closed this May 3, 2019
@dplewis dplewis deleted the fix/single-app-redirection branch May 3, 2019 21:26
@GoMino
Copy link

GoMino commented May 21, 2019

Can we hope for a new release with the fix? @dplewis

@TomWFox
Copy link
Contributor

TomWFox commented May 21, 2019

@GoMino I've opened a PR for a new release - #1083

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.

6 participants