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

feat!: core managed components functionality #279

Merged
merged 21 commits into from
Mar 11, 2024

Conversation

mmikita95
Copy link
Contributor

  • Moving Component, ComponentTree and SessionComponentTree classes to a separate ui_utils file.
  • StreamsyncUI class responsible for creating components and attaching them to ComponentTree
  • StreamsyncUIManager class intended as a "user-facing" interface
  • Tests for UI manager

- Moving Component, ComponentTree and SessionComponentTree classes to a separate ui_utils file.
- StreamsyncUI class responsible for creating components and attaching them to ComponentTree
- StreamsyncUIManager class intended as a "user-facing" interface
- Tests for UI manager
Satisfies default factory requirements for earlier versions of Pydantic
src/streamsync/ui_manager.py Outdated Show resolved Hide resolved
src/streamsync/ui_manager.py Outdated Show resolved Hide resolved
src/streamsync/ui_utils.py Outdated Show resolved Hide resolved
@ramedina86
Copy link
Collaborator

Looks good to me but I'd really prefer having @FabienArcellier 's review before merging

- Renaming ui_utils.py to core_ui.py
- Minor style fixes and unused imports removal
Copy link
Collaborator

@FabienArcellier FabienArcellier left a comment

Choose a reason for hiding this comment

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

Thanks, it's a great step forward.

We have to cover this pattern :

import streamsync as ss

with ss.init_ui() as ui:
  with ui.Page("other page"):
    ui.Text(text='Hello Pigeons')

tests/test_ui.py Outdated Show resolved Hide resolved
src/streamsync/ui_manager.py Show resolved Hide resolved
tests/test_ui.py Show resolved Hide resolved
tests/test_ui.py Outdated Show resolved Hide resolved
src/streamsync/ui_manager.py Show resolved Hide resolved
src/streamsync/ui_manager.py Show resolved Hide resolved
src/streamsync/ui.py Outdated Show resolved Hide resolved
src/streamsync/ui.py Outdated Show resolved Hide resolved
tests/test_ui.py Outdated Show resolved Hide resolved
tests/test_ui.py Outdated Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

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

question: is this intended?
class StreamsyncUI is in ui_manager.py
and class StreamsyncUIManager in in ui.py

Copy link
Contributor Author

@mmikita95 mmikita95 Mar 7, 2024

Choose a reason for hiding this comment

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

I see how this can be confusing. My logic was that ui_manager contains an "actual manager class" (StreamsyncUI), and ui provides easy access to the interface (from streamsync.ui import StreamsyncUIManager) that user is supposed to build upon. Hence the discrepancy in the class names and file names – ui_manager is reserved for internal use, and ui is "out-facing", while the class names are vice versa, so that StreamsyncUIManager could give a user a better hint on what it is supposed to do.
I think we might change that though, because, after contemplating on it, doesn't seem like the best choice.

src/streamsync/core.py Outdated Show resolved Hide resolved
@ramedina86 ramedina86 merged commit b53161f into writer:dev Mar 11, 2024
15 checks passed
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.

4 participants