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

Cockpit should have a tutorial #1286

Conversation

ArturoManzoli
Copy link
Contributor

@ArturoManzoli ArturoManzoli commented Sep 2, 2024

  • Added a tutorial that can be further converted into a reusable component;
  • Tutorial display flag is bound to the user configs;
  • It can be accessed on the Configurations > General -> User settings;
  • Can be disabled/re-enabled at its first and last step;

When testing, start with the vehicle disconnected, with a wrong IP for example.

image

image

Fix #1012

@rafaellehmkuhl
Copy link
Member

Will review it right now. Sorry for the delay.

src/App.vue Outdated
Comment on lines 4 to 10
<div
v-if="
!widgetStore.editingMode &&
!interfaceStore.showMainMenu &&
interfaceStore.mainMenuStyleTrigger === 'center-left'
"
>
Copy link
Member

Choose a reason for hiding this comment

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

We did raise the eslint rule for line chars right? Is it still giving us trouble?

Copy link
Contributor Author

@ArturoManzoli ArturoManzoli Sep 11, 2024

Choose a reason for hiding this comment

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

We did, but it seems to be an issue between code formatters. It will be addressed in a future task.

Comment on lines 55 to 72
setHighlightedComponent(component: string) {
this.componentToHighlight = component
},
setMainMenuVisibility(value: boolean) {
this.mainMenuVisibility = value
},
setMainMenuStep(step: number) {
this.mainMenuCurrentStep = step
},
setCurrentConfigComponent(componentIndex: number) {
this.configComponent = componentIndex
},
setKeepGlassModalAlwaysOnTop(value: boolean) {
this.isGlassModalAlwaysOnTop = value
},
setTutorialVisibility(value: boolean) {
this.isTutorialVisible = value
},
Copy link
Member

Choose a reason for hiding this comment

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

Are those methods needed?

One of Pinia's advantages is that it doesn't need mutation methods, and one can easily set the values from both inside and outside the store directly.

Unless we need the methods to create side-effects (like we do when we set the widgets view index for example, where we need to confirm if the index is valid), I think we can get rid of them.

Copy link
Contributor Author

@ArturoManzoli ArturoManzoli Sep 11, 2024

Choose a reason for hiding this comment

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

You are correct.
Those methods should exist in Pinia, only when some extra logic has to be applied on a state change.
Changing that..

@@ -182,7 +204,7 @@
:selected="false"
@click="
() => {
mainMenuStep = 1
Copy link
Member

Choose a reason for hiding this comment

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

It seems to me that the wizard logic got very coupled with the main-menu logic, inside this component file, specially with the currentConfigMenuComponent variable.

We can follow with this for now, but it would be important for the future to uncouple than, specially since the wizard will also be used for other parts of the application, like the edit mode and the toolbars.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with you.
The idea is to design easy to reuse (but not yet completely generic) components until they need to be reused. This way we save coding time until the component has an real need to be re-instantiated along the app.

Once the wizard is in fact reused, its code will change a bit to be more generic and this coupling will no longer exists.

@@ -152,4 +156,8 @@ const interfaceStore = useAppInterfaceStore()
const updateOpacity = (value: number): void => {
interfaceStore.setBgOpacity(value)
}

const resetColorsToDefault = (): void => {
interfaceStore.setUIGlassEffect(0.8, '#4F4F4F1A', '#FFFFFF', 25)
Copy link
Member

Choose a reason for hiding this comment

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

It would be good to interesting to concentrate those default values in the defaults.ts file, so it gets easy to find and anyone can import from there if needed.

@rafaellehmkuhl
Copy link
Member

rafaellehmkuhl commented Sep 9, 2024

A couple more things:

  1. The Cockpit logo color is changed. We should stick with the correct design to support brand strength. PS: after refreshing Cockpit it seems that it is green again. Maybe I didn't give a refresh after checking out the branch and it took an old CSS?
image
  1. In most of the tutorial pages, there's something blinking below the tutorial modal, and not in the configuration pages themselves. Seems like a bug, but it only happened to me the first time. In the next refreshes, the highlight seems in place. Maybe the git branch problem again?
image
  1. When I have an user set already, the tutorial works fine, but if I have a brand new Cockpit installation, without a user set nor a vehicle connected, the tutorial modal keeps showing up on every Cockpit refresh, even after I connected to a vehicle and set the user.

@ArturoManzoli
Copy link
Contributor Author

2. In most of the tutorial pages, there's something blinking below the tutorial modal,

Maybe is the H from the home location on the map. Are you in a house near Daniela Beach :)?
In my case the 'H' home is pretty close to where I live:
image

Check it out without the Wizard modal:
image

@rafaellehmkuhl
Copy link
Member

Maybe is the H from the home location on the map. Are you in a house near Daniela Beach :)?

Oooo it makes sense! My bad!

@ArturoManzoli
Copy link
Contributor Author

ArturoManzoli commented Sep 11, 2024

  1. The Cockpit logo color is changed. We should stick with the correct design to support brand strength. PS: after refreshing Cockpit it seems that it is green again. Maybe I didn't give a refresh after checking out the branch and it took an old CSS?

It was actually intentional, to differentiate the connected and disconnected state of the vehicle. But I agree with you. Changing the Cockpit logo color that dramatically wasn't right. We indeed should keep a branding consistency.

I reverted the color to the original green, but made it a little bit darker when the vehicle isn't connected:

Disconnecetd:
image
image

Connected:
image
image

@ArturoManzoli
Copy link
Contributor Author

3. When I have an user set already, the tutorial works fine, but if I have a brand new Cockpit installation, without a user set nor a vehicle connected, the tutorial modal keeps showing up on every Cockpit refresh, even after I connected to a vehicle and set the user.

I couldn't replicate the issue here. Maybe is something related to the flow. Have you pressed 'Don't show again'?
image

@rafaellehmkuhl
Copy link
Member

  1. When I have an user set already, the tutorial works fine, but if I have a brand new Cockpit installation, without a user set nor a vehicle connected, the tutorial modal keeps showing up on every Cockpit refresh, even after I connected to a vehicle and set the user.

I couldn't replicate the issue here. Maybe is something related to the flow. Have you pressed 'Don't show again'?

Didn't click the "don't show again" button, but even so, the problem is the opposite (it keeps showing again on every boot).

tutorial-bug.mp4

@ArturoManzoli ArturoManzoli force-pushed the 1012-Cockpit-should-have-a-tutorial branch from b8ea328 to 3cf00ad Compare September 11, 2024 17:17
@ArturoManzoli
Copy link
Contributor Author

ArturoManzoli commented Sep 11, 2024

Didn't click the "don't show again" button, but even so, the problem is the opposite (it keeps showing again on every boot).

I see now what was causing the confusion.
Now instead of the 'Don't show again' button on the last wizard step, there is a 'Previous' button; and the 'Close' button was replaced by a 'Finish' one, that also changes the 'cockpit-has-seen-tutorial' flag to true;

image

@rafaellehmkuhl
Copy link
Member

Didn't click the "don't show again" button, but even so, the problem is the opposite (it keeps showing again on every boot).

I see now what was causing the confusion. Now instead of the 'Don't show again' button on the last wizard step, there is a 'Previous' button; and the 'Close' button was replaced by a 'Finish' one, that also changes the 'cockpit-has-seen-tutorial' flag to true;

Nice! I actually think that the variable should be changed as soon as this last step is reached, so even if the user closes the dialog in any other way, or even refresh the application (?), the tutorial is seen already and won't re-appear.

@ArturoManzoli ArturoManzoli force-pushed the 1012-Cockpit-should-have-a-tutorial branch from 3cf00ad to f656a94 Compare September 11, 2024 18:24
@ArturoManzoli
Copy link
Contributor Author

Nice! I actually think that the variable should be changed as soon as this last step is reached, so even if the user closes the dialog in any other way, or even refresh the application (?), the tutorial is seen already and won't re-appear.

Done

Copy link
Member

@rafaellehmkuhl rafaellehmkuhl left a comment

Choose a reason for hiding this comment

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

All good now!

Great addition!

@rafaellehmkuhl rafaellehmkuhl merged commit 91cd347 into bluerobotics:master Sep 11, 2024
10 checks passed
@ES-Alexander ES-Alexander added the docs-needed Change needs to be documented label Sep 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs-needed Change needs to be documented
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cockpit should have a wizard/tutorial
3 participants