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

Introduce ui.teleport element #3159

Merged
merged 16 commits into from
Jun 6, 2024

Conversation

CrystalWindSnake
Copy link
Contributor

This PR introduces a ui.teleport component to NiceGUI, mirroring the functionality of Vue's Teleport. It is the key for enabling the direct creation of any NiceGUI component within table cell slots(#3148).

@rodja rodja requested a review from falkoschindler June 2, 2024 02:55
Copy link
Contributor

@falkoschindler falkoschindler left a comment

Choose a reason for hiding this comment

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

Thanks a lot for this pull request, @CrystalWindSnake! The teleport element feels pretty magical!

Here are some thoughts about it:

  • The target container is specified using a CSS select. I guess this is helpful when addressing an HTML element like in the demo. But sometimes you might want to use a NiceGUI element like in the pytests. Could we support both, CSS strings and references to a UI element?

  • The force_update method feels a bit like the existing update method. Do you think we could simply re-use update()? Otherwise I fear users will call update() and wonder why the content doesn't appear in the target container.

  • You added the ui.teleport element to the documentation section about control element. I'd suggest to move it to the section about page layout, since it isn't limited to control elements and it is basically a very powerful layouting feature.

  • I noticed that the teleported content in disappears when reloading the page. This is ok on individual pages, but confusing on the auto-index page. Do you think there's anything we can do about that?

@CrystalWindSnake
Copy link
Contributor Author

@falkoschindler Thank you for your help.

  • we should be able to construct appropriate CSS selectors using element.id.
  • force_update() is used in scenarios where the target container is destroyed and rebuilt, which might be different from the scenarios for update().Additionally, force_update forces a re-render of teleport by updating the key, but I don't understand how this differs from props updates. I'm concerned it might cause other issues . However, I respect your decision and defer to your judgment .
  • Agreed. it is more appropriate to classify the ui.teleport component as a layout element.
  • I found a solution from the Vue repository, but I don't understand the reason behind it. The issue is indeed resolved.

@falkoschindler falkoschindler added the enhancement New feature or request label Jun 6, 2024
@falkoschindler falkoschindler added this to the 1.4.27 milestone Jun 6, 2024
@falkoschindler falkoschindler self-requested a review June 6, 2024 06:35
Copy link
Contributor

@falkoschindler falkoschindler left a comment

Choose a reason for hiding this comment

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

Thanks again, @CrystalWindSnake!
Looks like the PR is ready to merge. 🚀

@falkoschindler falkoschindler merged commit c30752a into zauberzeug:main Jun 6, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants