Skip to content

fixes #1881: Open generatorUrl in a new browser tab. #2470

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dpryden
Copy link

@dpryden dpryden commented Jan 29, 2021

There does not appear to be a good way to reliably determine in
onUrlRequest whether the URL being navigated to is an Internal or
External URL, especially if Alertmanager is hosted on the same domain as
other services (for example, Prometheus). To workaround this problem for
the specific case of the generatorUrlButton (labeled "Source"), we force
it to have target="_blank", which prevents Elm from intercepting the URL
and trying to do client-side navigation.

Signed-off-by: Daniel Pryden [email protected]

@dpryden
Copy link
Author

dpryden commented Jan 29, 2021

This PR is supposed to be associated with #1881 but it doesn't seem to be linked, I'm not sure if I did something wrong when creating the PR.

@hooten
Copy link
Contributor

hooten commented Feb 6, 2021

The easiest way to link a PR to an issue is using a keyword in the PR description.

@dpryden
Copy link
Author

dpryden commented Feb 6, 2021

The easiest way to link a PR to an issue is using a keyword in the PR description.

Thank you for your response, @hooten. Doesn't the commit message title contain a properly formatted keyword? That was my intention by writing fix #1881 at the start of the message.

@dpryden dpryden changed the title fix #1881: Open generatorUrl in a new browser tab. fix prometheus/alertmanager#1881: Open generatorUrl in a new browser tab. Feb 6, 2021
@dpryden dpryden changed the title fix prometheus/alertmanager#1881: Open generatorUrl in a new browser tab. fixes #1881: Open generatorUrl in a new browser tab. Feb 6, 2021
@hooten
Copy link
Contributor

hooten commented Feb 7, 2021

@dpryden my bad! You're right. It looks like the commit message did in fact link the issue. Sorry for the confusion

@stale stale bot added the stale label Apr 10, 2021
@JohanElmis
Copy link

JohanElmis commented Apr 14, 2021

I'm having the same issue where I have a nginx frontend and proxies to alertmanager or different prometheus-hosts based on path.
The Alertmanager Source-link for an Alert just reloads the page as it is now, despite that the link is correct (works if copied and opened in new tab). This new-tab solution will work well for me.

@stale stale bot removed the stale label Apr 14, 2021
@stale stale bot added the stale label Jun 13, 2021
@stale stale bot removed the stale label Jul 17, 2021
@stale stale bot added the stale label Sep 19, 2021
@tommyk
Copy link

tommyk commented Oct 17, 2022

would love to see this issue get fixed. can we fix conflicts and merge? i can create another PR if this has been abandoned.

@dpryden
Copy link
Author

dpryden commented Oct 17, 2022

Sorry, I haven't looked at this PR in ages, I actually forgot it was still open. I just rebased it and regenerated the asset/assets_vfsdata.go, I think it should be up to date now.

@dpryden
Copy link
Author

dpryden commented Oct 17, 2022

@simonpasquier, you were the one who added the help wanted label on #1881 -- are you able to help review this?

There does not appear to be a good way to reliably determine in
onUrlRequest whether the URL being navigated to is an Internal or
External URL, especially if Alertmanager is hosted on the same domain as
other services (for example, Prometheus). To workaround this problem for
the specific case of the generatorUrlButton (labeled "Source"), we force
it to have target="_blank", which prevents Elm from intercepting the URL
and trying to do client-side navigation.

Signed-off-by: Daniel Pryden <[email protected]>
@kerma
Copy link

kerma commented Feb 14, 2023

Thanks for the fix @dpryden! @simonpasquier any way to get this reviewed/merged?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants