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

feat: initial onboarding implementation - podman installation #3308

Merged
merged 15 commits into from Aug 3, 2023

Conversation

lstocchi
Copy link
Contributor

@lstocchi lstocchi commented Jul 21, 2023

What does this PR do?

This is the initial implementation for the onboarding feature.
It only covers a simple onboarding for podman. Check system requirements -> install podman -> yay! done.

The onboarding can only be started in the podman extension page so far. I'll refactor it in next PR. Starting it from the create new.. button in the resources page or when PD gets started was a bit more complex and i'd like to keep this PR simple and focus on the workflow.
BTW, the onboarding button will only be visible if the experimental onboarding setting (hidden by default) is set to true.
So a user who wants to try it should edit the settings file manually.
A normal user will never see this or access to the onboarding.

In the video below i show what happens when you follow the workflow and it fails somehow, what happens if it succeed, the messages that popup to ask the user to restart the onboarding, the cancel and skip buttons

https://drive.google.com/file/d/1dxcuzc8gzUyoe_wh_DAVrvgIIsi4Z5Jx/view?usp=sharing

In this second video i show how the errors are displayed if the system requirements step fails somehow (i had to fake it by updating the code).

https://drive.google.com/file/d/1TQnpdW_0o6wSf_vSzlfjbV64k3AqrraG/view?usp=sharing

Everything is created by reading the package.json file

Screenshot/screencast of this PR

https://drive.google.com/file/d/1dxcuzc8gzUyoe_wh_DAVrvgIIsi4Z5Jx/view?usp=sharing

https://drive.google.com/file/d/1TQnpdW_0o6wSf_vSzlfjbV64k3AqrraG/view?usp=sharing

What issues does this PR fix or reference?

#3172

How to test this PR?

  1. uninstall podman
  2. set the onboarding setting to true (go to .local\share\containers\podman-desktop\configuration\settings.json and add "experimental.onboarding":true)
  3. start PD, go to settings, extension , podman , click on the onboarding button

I still have to write some test for the onboarding.svelte file but you can play with it, suggest changes, etc..

@lstocchi lstocchi force-pushed the i3172 branch 4 times, most recently from 47edc14 to dd4091a Compare July 25, 2023 14:13
@lstocchi lstocchi marked this pull request as ready for review July 25, 2023 14:18
@lstocchi lstocchi requested review from a team and benoitf as code owners July 25, 2023 14:18
@lstocchi lstocchi requested review from cdrage, mairin and deboer-tim and removed request for a team July 25, 2023 14:18
@mairin
Copy link
Member

mairin commented Jul 25, 2023

I haven't tested yet, but the screencasts look awesome!! 😍

@lstocchi lstocchi marked this pull request as draft July 27, 2023 13:26
@lstocchi
Copy link
Contributor Author

Converting to draft as i'm simplifying the json after some suggestions received by Florent

@lstocchi lstocchi marked this pull request as ready for review August 1, 2023 12:01
@lstocchi
Copy link
Contributor Author

lstocchi commented Aug 1, 2023

@benoitf i guess i've finished simplifying the json + refactor the code. I still need to add tests for the svelte components but you can give it a look/try when you have time

extensions/podman/src/extension.ts Outdated Show resolved Hide resolved
extensions/podman/package.json Show resolved Hide resolved
extensions/podman/package.json Outdated Show resolved Hide resolved
packages/main/src/plugin/extension-loader.ts Outdated Show resolved Hide resolved
@lstocchi lstocchi requested a review from benoitf August 1, 2023 14:20
@lstocchi lstocchi requested a review from benoitf August 2, 2023 08:02
@lstocchi lstocchi requested a review from benoitf August 2, 2023 08:57
@benoitf
Copy link
Collaborator

benoitf commented Aug 2, 2023

I've tested and it works well

I've just a remark (but anyway it's not blocking this PR), just for open discussions.

At the last step:
image

I'm wondering if the button should not be done instead of next as I'm at the end of the flow.
I think it's more for @mairin

Another remark is where 'onboarding' button is present
I think it's not obvious (I know it's like a temporary location) as it's well hidden.

image

so I'm wondering if it could not be part of the Resource screen
image

Also if I click on 'skip' item I have
image

saying it's configurable from 'resources' screen

(it's not blocking to merge the PR)

Another remark will be about how it scale (like multiple steps, labels too long)

image

(here current step is not centered)

image

Another remark about the chevron icon
image

What does it mean ?

And last remark, this one as well more for @mairin

image

I'm not convinced 100% about the current step being a purple circle
I feel like it's not fitting with other circles where it's not solid icons. I don't know if it's because there is no white circle around, if it's because it's a plain circle, if I would expect another icon inside the circle, etc. It's hard to describe.
Maybe it's because previous steps/circles are not purple as well (like completed)

I like when they're connecting lines as well, here is an example
image
image

image

@lstocchi
Copy link
Contributor Author

lstocchi commented Aug 2, 2023

Cool, i agree with most of the remarks as it is something i noted as well and i wanted to talk with Mo to know how to fix them.

Just a note. I temporarily added a button to test it (not visible by default, the user needs to manually set a configuration in the PD settings file to see/activate the onboarding). When i discussed with Mo, we talked about activating the onboarding when clicking on the Create new... button if podman was not install or first time PD is opened.
I opted for this temporary solution to be quicker but I'll work on them in separate PRs.

Regarding all the rest, if there is no strong blocker i'd like to merge this as it is (it's hidden by default) and then fix all further problems in separate PRs, like PR -> center labels on top right when having multiple items, PR -> fix icons, PR -> handle when to enable onboarding.
So to not increase the size of this PR further.

Another remark about the chevron icon image

What does it mean ?

I used it as a temporary icon to show that a step has been skipped. I didn't find anything in the mockup. I need some suggestion by Mo.

@benoitf
Copy link
Collaborator

benoitf commented Aug 2, 2023

While testing the flow I also found this behaviour when aborting the install process

image

it looks like a success in icons but I feel it should be a 'success' or I should have a retry

@lstocchi lstocchi force-pushed the i3172 branch 2 times, most recently from 68f47d7 to d3670ee Compare August 2, 2023 10:38
@lstocchi
Copy link
Contributor Author

lstocchi commented Aug 2, 2023

While testing the flow I also found this behaviour when aborting the install process

it looks like a success in icons but I feel it should be a 'success' or I should have a retry

You mean 'failed' in the install podman step?

The problem is that we only check if the completionEvents have been satisfied to proceed to the next step. So we only know if a step is running or completed. But when completed we still don't have a way to differentiate if it has been completed with a failure or it was successfully.

@benoitf
Copy link
Collaborator

benoitf commented Aug 2, 2023

yes I would expect that either flow is in 'interrupted' state or that I have a cross/X icon and not saying that 'install podman' has a checked icon

Also from there I can't retry the install as I'm already at the next step. Probably I should not be able to move from the step to the next one if this step is not successful.

I am thinking that for example if I have clicked on exit on the installer by mistake I would want to retry the failed step and or having abort

(of course it can be followed up PRs, just discussing the current flow)

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.

approving as first iteration

then we'll probably to have sub-issues to discuss feedback/improve current flow

@benoitf
Copy link
Collaborator

benoitf commented Aug 2, 2023

nice work @lstocchi 🚀 🦭

Signed-off-by: lstocchi <lstocchi@redhat.com>
Signed-off-by: lstocchi <lstocchi@redhat.com>
Signed-off-by: lstocchi <lstocchi@redhat.com>
Signed-off-by: lstocchi <lstocchi@redhat.com>
Signed-off-by: lstocchi <lstocchi@redhat.com>
Signed-off-by: lstocchi <lstocchi@redhat.com>
Signed-off-by: lstocchi <lstocchi@redhat.com>
Signed-off-by: lstocchi <lstocchi@redhat.com>
Signed-off-by: lstocchi <lstocchi@redhat.com>
Signed-off-by: lstocchi <lstocchi@redhat.com>
Signed-off-by: lstocchi <lstocchi@redhat.com>
Signed-off-by: lstocchi <lstocchi@redhat.com>
Signed-off-by: lstocchi <lstocchi@redhat.com>
Signed-off-by: lstocchi <lstocchi@redhat.com>
Signed-off-by: lstocchi <lstocchi@redhat.com>
@lstocchi
Copy link
Contributor Author

lstocchi commented Aug 3, 2023

@mairin we decided to merge this as it is today bc it has a good core (json structure, ...) and it's a hidden feature, nobody will see it until everything is fixed.
I'll be off for the next 2 weeks so if you have any free time to give this a look in the meantime, feel free to comment or open new issues. I'll reply/bother you/work on it when i'm back 😆
Tomorrow i'll update the gif/video so you can see it better

Gif recorded
onboarding_podman

@lstocchi lstocchi merged commit 1131ed4 into containers:main Aug 3, 2023
8 checks passed
@podman-desktop-bot podman-desktop-bot added this to the 1.3.0 milestone Aug 3, 2023
@lstocchi lstocchi deleted the i3172 branch August 4, 2023 09:00
odockal pushed a commit to odockal/podman-desktop that referenced this pull request Aug 7, 2023
…ners#3308)

* feat: initial onboarding implementation - podman installation

Signed-off-by: lstocchi <lstocchi@redhat.com>

* fix: check configuration value before returning onboarding

Signed-off-by: lstocchi <lstocchi@redhat.com>

* fix: save status when completing onboarding + fix broken behavior

Signed-off-by: lstocchi <lstocchi@redhat.com>

* fix: lint and format

Signed-off-by: lstocchi <lstocchi@redhat.com>

* fix: clean code + some tests

Signed-off-by: lstocchi <lstocchi@redhat.com>

* fix: simplify json

Signed-off-by: lstocchi <lstocchi@redhat.com>

* fix: clean

Signed-off-by: lstocchi <lstocchi@redhat.com>

* feat: refactor how to save/retrieve onboarding context values

Signed-off-by: lstocchi <lstocchi@redhat.com>

* fix: fix tests

Signed-off-by: lstocchi <lstocchi@redhat.com>

* fix: use extensionInfo api to retrieve id

Signed-off-by: lstocchi <lstocchi@redhat.com>

* fix: add onboardingContext prefix for onboarding context values

Signed-off-by: lstocchi <lstocchi@redhat.com>

* fix: refactor to validate eslint rules

Signed-off-by: lstocchi <lstocchi@redhat.com>

* fix: add catch to executeCommand

Signed-off-by: lstocchi <lstocchi@redhat.com>

* fix: add check when converting/assigning image

Signed-off-by: lstocchi <lstocchi@redhat.com>

* chore: add tests

Signed-off-by: lstocchi <lstocchi@redhat.com>

---------

Signed-off-by: lstocchi <lstocchi@redhat.com>
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.

None yet

4 participants