Conversation
sfc-gh-bnisco
left a comment
sfc-gh-bnisco
left a comment
There was a problem hiding this comment.
Not yet done with reviewing this PR, but wanted to leave some feedback at this checkpoint. Will come back and go through the remaining docs soon.
content/develop/concepts/custom-components/components-v2/create.md
Outdated
Show resolved
Hide resolved
content/develop/concepts/custom-components/components-v2/create.md
Outdated
Show resolved
Hide resolved
content/develop/concepts/custom-components/components-v2/create.md
Outdated
Show resolved
Hide resolved
content/develop/concepts/custom-components/components-v2/create.md
Outdated
Show resolved
Hide resolved
content/develop/concepts/custom-components/components-v2/create.md
Outdated
Show resolved
Hide resolved
| For development, temporarily use the dev server URL: | ||
|
|
||
| ```python | ||
| # Development mode (temporary) | ||
| component = st.components.v2.component( | ||
| name="my_component", | ||
| js="http://localhost:5173/src/index.ts", # Dev server URL | ||
| data={"message": "Hello"} | ||
| ) | ||
| ``` |
There was a problem hiding this comment.
suggestion: This is incorrect, and feels like a relic of the V1 pattern. With the library build system we have set up in Vite (in the component-template repo), it doesn't run its own server. It still outputs just a file that is hashed, so js="index-*.js", is still the correct value here.
|
|
||
| ## Publishing your package | ||
|
|
||
| ### Build the distribution |
There was a problem hiding this comment.
suggestion: Similar feedback here with other sections in this doc, but I don't know how in-depth we should go here considering this isn't Streamlit-specific to publish a package to PyPI. People have their own opinionated ways of building and deploying, so might make our lives simpler to cut this and instead suggest that they can build, package, and deploy to PyPI.
Generally speaking, users who are going to build an entire CCv2 package-based component are already advanced users anyway, so this should be fine.
| interface ComponentProps { | ||
| data: MyComponentData; | ||
| setStateValue: (key: string, value: any) => void; | ||
| setTriggerValue: (key: string, value: any) => void; | ||
| parentElement: HTMLElement; | ||
| } |
There was a problem hiding this comment.
issue: Point folks to using the types from @streamlit/component-v2-lib. This suggests that they redefine the types that we've already provided for them in that package, which is not recommended.
| }; | ||
| } catch (error) { | ||
| console.error("Component error:", error); | ||
| component.parentElement.innerHTML = `<div class="error">Component failed to load</div>`; |
There was a problem hiding this comment.
suggestion: I wouldn't recommend this pattern as written, since this goes against our earlier best practice of not overwriting the entire parentElement.
Additionally, this wraps the entire component in a try/catch, which is also not a recommended pattern for any non-trivial component.
Instead of this specific example, I'd instead call out that error handling in the custom component is up to the author to handle.
content/develop/concepts/custom-components/components-v2/quickstart.md
Outdated
Show resolved
Hide resolved
content/develop/concepts/custom-components/components-v2/state-and-triggers.md
Show resolved
Hide resolved
content/develop/concepts/custom-components/components-v2/theming.md
Outdated
Show resolved
Hide resolved
content/develop/concepts/custom-components/components-v2/theming.md
Outdated
Show resolved
Hide resolved
content/develop/concepts/custom-components/components-v2/theming.md
Outdated
Show resolved
Hide resolved
|
@sfc-gh-bnisco Don't worry about the package-based development page yet. It's definitely not ready for review. 😅 |
2 participants
📚 Context
Conceptual guides for custom components v2.
Contribution License Agreement
By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.