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

core: frontend: Add generic popup library #2426

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

JoaoMario109
Copy link
Collaborator

Add a generic popup component to be used when user confirmation or interaction is needed

Linked to the solution used for #2406

* Instal vue final modal lib for generic modals
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.

Nice addition!
It would be cool to use it also on Cockpit!

@@ -49,6 +49,7 @@
"uuid": "^9.0.0",
"vue": "^2.6.11",
"vue-apexcharts": "^1.6.2",
"vue-final-modal": "legacy",
Copy link
Member

Choose a reason for hiding this comment

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

Interesting library!

@JoaoMario109
Copy link
Collaborator Author

I'll keep this as a draft for now because currently the popup modal is not consuming the click and keys events, so its leaking for underlying v-dialogs and mistakenly closing them

@JoaoMario109
Copy link
Collaborator Author

Added an issue in vue-final-modal maybe we can get some answers from there related to the events propagation problem

vue-final/vue-final-modal#435

Copy link
Member

@patrickelectric patrickelectric left a comment

Choose a reason for hiding this comment

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

There is also a lost .lock file

fire(options: PopupOptions): Promise<PopupResult> {
return new Promise<PopupResult>((resolve) => {
this.addPopup({
id: options.id as string,
Copy link
Member

Choose a reason for hiding this comment

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

why is this necessary ? isn't id a string already ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The ID is essentially a string. However, the popup options provide users with the flexibility to leave it unset because it is an optional property. If the user does not set it, the fire method within the Popup functionality automatically assigns a value to it. Consequently, when this data is passed to the addPopup method, it is treated as an ActivePopup. Within this context the ID is already defined either by user or fire method, so the conversion is only to remove the assignment from string | undefined from the PopupOption to the ActivePopup.

We could also change it on the Action fire to receive a direct id parameter that is already string and that will be defined in the fire method, in this way the compiler can check that is not undefined

core/frontend/src/store/popups.ts Outdated Show resolved Hide resolved
core/frontend/src/components/popups/Popup.vue Outdated Show resolved Hide resolved
core/frontend/src/components/popups/Popup.vue Outdated Show resolved Hide resolved
* Add a statically mounted popup library to allow launching stackable
  popups and get user interaction result
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.

3 participants