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: move content to the middle and redirect to podman desktop on page load event #75

Merged
merged 2 commits into from
Mar 6, 2024

Conversation

dgolovin
Copy link
Collaborator

@dgolovin dgolovin commented Mar 5, 2024

Fix #40. Cannot avoid opening page right now, but redirect on window load event works.

Open podman desktop on window load

Screen.Recording.2024-03-04.at.8.11.40.PM.mov

Open podman desktop using button on the page

Screen.Recording.2024-03-04.at.8.14.27.PM.mov

@dgolovin dgolovin requested a review from a team as a code owner March 5, 2024 04:07
@dgolovin dgolovin requested review from jeffmaury, cdrage and odockal March 5, 2024 04:07
Copy link
Collaborator

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

LGTM, tested successfully, thanks!

Waiting for @benoitf's code review before merging.

@dgolovin
Copy link
Collaborator Author

dgolovin commented Mar 5, 2024

I did not focus on how the page looks, should be follow up issue. I just moved it to middle page to avoid it to be covered when browser asks about using podman desktop to open the link.

Copy link
Member

@jeffmaury jeffmaury left a comment

Choose a reason for hiding this comment

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

Works for me but having handled the positive response in the local http server would allow better workflow

www/success.html Outdated
@@ -10,19 +10,31 @@
type="text/css"
href="https://unpkg.com/@patternfly/[email protected]/patternfly.min.css"
/>
<script lang="javascript">
function onLoad() {
window.location.href = 'podman-desktop:///preferences/authentication-providers'
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this URL is not handled/managed by Podman Desktop and I'm not sure it'll be this URL being used.

Either we use a shorter URL that is accepted/handled property
or we need to add inside Podman Desktop a way to handle /preferences/authentication-providers but in that case I think it might be missing a prefix

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I see now, basically I can have podman-desktop:/// and it would give the same result.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we need to add inside Podman Desktop a way to handle /preferences/authentication-providers but in that case I think it might be missing a prefix

Do you mean a prefix similar to podman-desktop://extension/ used in extension installation?

Would something like podman-desktop://goto/preferences/authentication-providers work? So we can translate it to router.goto('/preferences/authentication-providers')` call on podman desktop side.

Copy link
Collaborator

@benoitf benoitf left a comment

Choose a reason for hiding this comment

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

as side comments, I was wondering why the copyright of the css file is Microsoft

/*---------------------------------------------------------------------------------------------
 *  Copyright (c) Microsoft Corporation. All rights reserved.
 *  Licensed under the MIT License. See License.txt in the project root for license information.
 *--------------------------------------------------------------------------------------------*/

and about the page, I think it would be better if the page would be self-contained/minimized/production-ready b/c here we're fetching from internet the patternfly library for example

(follow-up issues)

I would just make the podman-desktop:// redirect link shorter as the link being used is not implemented and many not be implemented like that in the future

@dgolovin
Copy link
Collaborator Author

dgolovin commented Mar 6, 2024

@benoitf can I push it for now, then open all follow ups?

@benoitf
Copy link
Collaborator

benoitf commented Mar 6, 2024

@dgolovin please use podman-desktop:// for now and you can merge with that

www/success.html Outdated
@@ -10,19 +10,31 @@
type="text/css"
href="https://unpkg.com/@patternfly/[email protected]/patternfly.min.css"
/>
<script lang="javascript">
function onLoad() {
window.location.href = 'podman-desktop:///preferences/authentication-providers'
Copy link
Collaborator

Choose a reason for hiding this comment

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

@dgolovin I think this can be discussed in follow-up

IMHO I think we need to ask to redirect to an extension page/command then it's the responsibility of the extension to do whatever the extension want. (There are some navigation API available for the extension)

But for now I think it's OK to just asking to bring back the main window of Podman Desktop on top by just asking podman-desktop://

@vrothberg
Copy link
Collaborator

Testing now with the latest main from Podman Desktop (i.e., commit 287c15d429c1):
When clicking on sign in, the tab will open in the browser but the focus is still on Podman Desktop not on the tab.

Can others reproduce?

@benoitf
Copy link
Collaborator

benoitf commented Mar 6, 2024

@vrothberg looks like I've got the expected behavior on my side (focus on Chrome then Podman Desktop)

Screen.Recording.2024-03-06.at.13.46.24.mov

I used the same sha

$ git log -1 | head -1                                                                                                                                                                                                                                   
commit 287c15d429c1dd104c3451012ccce9e37d50da45

@dgolovin
Copy link
Collaborator Author

dgolovin commented Mar 6, 2024

@vrothberg same for me, it works as expected, browser gets focus.

@dgolovin dgolovin merged commit 7aea86f into redhat-developer:main Mar 6, 2024
4 checks passed
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.

Avoid the success page and automatically redirect the user in Podman Desktop
4 participants