Skip to content

feat(vue): support for showModal #1872

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 13 commits into
base: main
Choose a base branch
from

Conversation

tiagogviegas
Copy link
Contributor

@tiagogviegas tiagogviegas commented May 8, 2025

💡 What is the current behavior?

Showing an Modal on Vue differs from other frameworks and has no support for the showModal method.

GitHub Issue: Fixes #1771

🆕 What is the new behavior?

  • Added support for showModal to Vue
  • Changed to Vite (current recommend way) to build the vue lib (needed because of custom SFC vue components)

🏁 Checklist

A pull request can only be merged if all of these conditions are met (where applicable):

  • 🦮 Accessibility (a11y) features were implemented
  • 🗺️ Internationalization (i18n) - no hard coded strings
  • 📲 Responsiveness - components handle viewport changes and content overflow gracefully
  • 📄 Documentation was reviewed/updated (pnpm run docs)
  • 🧪 Unit tests were added/updated and pass (pnpm test)
  • 📸 Visual regression tests were added/updated and pass (Guide)
  • 🧐 Static code analysis passes (pnpm lint)
  • 🏗️ Successful compilation (pnpm build, changes pushed)

👨‍💻 Help & support

Copy link

changeset-bot bot commented May 8, 2025

🦋 Changeset detected

Latest commit: d8b3ad1

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 4 packages
Name Type
@siemens/ix-vue Minor
@siemens/ix Minor
@siemens/ix-angular Minor
@siemens/ix-react Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@tiagogviegas tiagogviegas force-pushed the vue/modal branch 9 times, most recently from 5f4656a to 57731d4 Compare May 9, 2025 10:03
@danielleroux
Copy link
Collaborator

@tiagogviegas Thank for contribution great work there 👍! We are doing the review asap, we have to sync the recent activities regarding vue with your PR.

@danielleroux
Copy link
Collaborator

@tiagogviegas With PR #1883 we moving our documentation to https://github.com/siemens/ix-docs.

The PR contains some documentation changes, we have two options.

Option 1: You rise a PR regarding the docu changes inside the new repo
Option 2: I will add the changes directly to the documentation after your pr is reviewed/merged?

I would suggest Option 2, less work for you to keep the PRs up 2 date. Just give me a short answer

@tiagogviegas
Copy link
Contributor Author

tiagogviegas commented May 16, 2025

@tiagogviegas With PR #1883 we moving our documentation to https://github.com/siemens/ix-docs.

The PR contains some documentation changes, we have two options.

Option 1: You rise a PR regarding the docu changes inside the new repo Option 2: I will add the changes directly to the documentation after your pr is reviewed/merged?

I would suggest Option 2, less work for you to keep the PRs up 2 date. Just give me a short answer

@danielleroux Sure let's go with option 2. Thanks for review it 😄

@danielleroux
Copy link
Collaborator

@tiagogviegas With PR #1883 we moving our documentation to https://github.com/siemens/ix-docs.
The PR contains some documentation changes, we have two options.
Option 1: You rise a PR regarding the docu changes inside the new repo Option 2: I will add the changes directly to the documentation after your pr is reviewed/merged?
I would suggest Option 2, less work for you to keep the PRs up 2 date. Just give me a short answer

@danielleroux Sure let's go with option 2. Thanks for review it 😄

Your changes are added to the new PR here: siemens/ix-docs#7

After the #1883 is merged you will get merge conflicts regarding packages/documentation you can just take the incoming main. Thank you!

@danielleroux
Copy link
Collaborator

danielleroux commented May 22, 2025

I have tested it with the vue-test-app. I found that the vue test app behave different to the other test apps.
e.g react-test-app
https://github.com/user-attachments/assets/89f6225a-7958-4ba2-aace-de148518df26
(sorry you have to download mov for some reason github preview function of screensharing is not working)

by default:

  • backdrop is visible
  • animation is shown
  • ... other default properties

It looks like the properties of the modal element are not being applied correctly.

@tiagogviegas
Copy link
Contributor Author

I have tested it with the vue-test-app. I found that the vue test app behave different to the other test apps. e.g react-test-app https://github.com/user-attachments/assets/89f6225a-7958-4ba2-aace-de148518df26 (sorry you have to download mov for some reason github preview function of screensharing is not working)

by default:

  • backdrop is visible
  • animation is shown
  • ... other default properties

It looks like the properties of the modal element are not being applied correctly.

@danielleroux Fixed! Totally missed that.

@tiagogviegas tiagogviegas requested a review from danielleroux May 22, 2025 08:44
Copy link

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.

[ix-model] modal not displaying as expected in VUE
2 participants