Skip to content

feat(dialogs/spawnDialog): separate spawning options from dialog props and allow Element as a container #6756

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

Merged
merged 2 commits into from
Apr 10, 2025

Conversation

ShGKme
Copy link
Contributor

@ShGKme ShGKme commented Apr 8, 2025

☑️ Resolves

🏁 Checklist

  • ⛑️ Tests are included or are not applicable
  • 📘 Component documentation has been extended, updated or is not applicable
  • 2️⃣ Backport to stable8 for maintained Vue 2 version or not applicable

@ShGKme ShGKme added enhancement New feature or request 3. to review Waiting for reviews feature: functions composables, functions, mixins and other non-components labels Apr 8, 2025
@ShGKme ShGKme added this to the v9.0.0-rc.0 milestone Apr 8, 2025
@ShGKme ShGKme requested a review from susnux April 8, 2025 14:30
@ShGKme ShGKme self-assigned this Apr 8, 2025
Copy link
Contributor

@susnux susnux left a comment

Choose a reason for hiding this comment

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

non breaking change as such I approve. But I would really favorite to move the onClose callback to a promise based return.

@ShGKme ShGKme requested review from st3iny and Antreesy April 8, 2025 15:54
@ShGKme ShGKme requested review from DorraJaouad and skjnldsv April 10, 2025 10:54
Copy link
Contributor

@Antreesy Antreesy left a comment

Choose a reason for hiding this comment

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

Looks valid.
But I don't get what's the reason for moving container out? it should be delegated to Vue on where to mount component instead of 'appendChild'?

@ShGKme
Copy link
Contributor Author

ShGKme commented Apr 10, 2025

But I don't get what's the reason for moving container out? it should be delegated to Vue on where to mount component instead of 'appendChild'?

Do you mean why we create a new element inside the container and not mount directly into the container?

@ShGKme
Copy link
Contributor Author

ShGKme commented Apr 10, 2025

If yes - because it allows using container for many dialogs and have other elements there.

For example, if you pass container: 'body', you probably expect that the dialog will be mounted somewhere at the end of the body. And you don't expect that entire body content will be replaced with the dialog only.

let { container } = optionsOrOnClose

// For backwards compatibility try to use container from props
if ('container' in props && typeof props.container === 'string') {
Copy link
Contributor

@susnux susnux Apr 10, 2025

Choose a reason for hiding this comment

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

Suggested change
if ('container' in props && typeof props.container === 'string') {
if (!container && 'container' in props && typeof props.container === 'string') {

Do not override if already in opts

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did

if ('container' in props && typeof props.container === 'string') {
	container ??= props.container
}

instead.

(Seems slightly more readable to me to not think about brackets in !container && 'container' in props)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

!container && 'container' in props vs !container && ('container' in props)

@ShGKme ShGKme force-pushed the fix/spawnDialog--props-options branch from d499f4d to c726508 Compare April 10, 2025 12:54
@ShGKme ShGKme enabled auto-merge April 10, 2025 12:56
@ShGKme ShGKme merged commit b6b24ec into main Apr 10, 2025
28 checks passed
@ShGKme ShGKme deleted the fix/spawnDialog--props-options branch April 10, 2025 12:58
@susnux
Copy link
Contributor

susnux commented Apr 10, 2025

/backport to stable8

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews enhancement New feature or request feature: functions composables, functions, mixins and other non-components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants